Allow named selector to not do partial matching

[andriokha]

Hi, thanks for all the work over the years on this! ❤️

I searched and couldn't find a matching issue (tho #872 covers the same area of the codebase). I'd love to be able to choose to have strict matching when using the named selector (ie named behaves identically to named_exact). It seems currently that the behavior's hardcoded:

Mink/src/Element/ElementFinder.php

Lines 48 to 57 in 9b08f62

	public function findAll(string $selector, $locator, string $parentXpath)
	{
	if ('named' === $selector) {
	$items = $this->findAll('named_exact', $locator, $parentXpath);
	if (empty($items)) {
	$items = $this->findAll('named_partial', $locator, $parentXpath);
	}
	return $items;
	}

And the element finder's instantiated directly in the session constructor:

Mink/src/Session.php

Lines 42 to 50 in 9b08f62

	public function __construct(DriverInterface $driver, ?SelectorsHandler $selectorsHandler = null)
	{
	$this->driver = $driver;
	$this->selectorsHandler = $selectorsHandler ?? new SelectorsHandler();
	$this->elementFinder = new ElementFinder($driver, $this->selectorsHandler);
	$this->page = new DocumentElement($this);
	$driver->setSession($this);
	}

From my perspective it'd be great to either have a way to override the element finder with my own custom class, or to update the default Mink finder to support switching off fallback partial matching.

Do either of those approaches sound like something you'd consider? If not, is there maybe some other solution?

Thanks!

[aik099]

I see the potential in making a default selector name (the named currently) configurable. Unsure where this configuration should take place:

• the new $locator = 'named' parameter in TraversableElement class;
• a session level setting that is inherited by the DocumentElement (aka page) and each NodeElement it produces;
• anything else.

[andriokha]

Sorry for the delay in getting back. Another possible option would be to allow named to be registered. It's not super clean, but it would be a relatively minimal change:

--- a/src/Element/ElementFinder.php
+++ b/src/Element/ElementFinder.php
@@ -47,7 +47,7 @@ class ElementFinder
      */
     public function findAll(string $selector, $locator, string $parentXpath)
     {
-        if ('named' === $selector) {
+        if ('named' === $selector && !$this->selectorsHandler->isSelectorRegistered('named')) {
             $items = $this->findAll('named_exact', $locator, $parentXpath);
             if (empty($items)) {
                 $items = $this->findAll('named_partial', $locator, $parentXpath);

Otherwise I'd consider having it session-level. I don't know the codebase well, so take my suggestions with a pinch of salt :)

We'll need to be careful about where named is currently used. For example the following ought to be changed to named_partial if I understand correctly, otherwise it'll only return true if the whole document text equals $content:

Mink/src/Element/DocumentElement.php

Lines 40 to 50 in 9b08f62

	/**
	* Check whether document has specified content.
	*
	* @param string $content
	*
	* @return bool
	*/
	public function hasContent(string $content)
	{
	return $this->has('named', array('content', $content));
	}

Thanks!

[aik099]

I've researched the code a bit more:

The ElementFinder could be the right place for this setting, not Session. Every Element/NodeElement (no matter how deep) ends up holding a reference to the same ElementFinder instance (in master branch (1.x) that's transitively via the shared Session; on 2-architecture-changes branch (2.x) it's even more direct — ElementFinder::findAll() constructs new NodeElements passing $this as their finder). So if we put a setting on ElementFinder, every element just sees it automatically — no need to pass anything through TraversableElement's methods or Element's constructor.

Concretely: ElementFinder could hold a simple mode — 'named' (today's exact-then-partial fallback, unchanged default), 'named_exact', or 'named_partial' — checked at the top of the existing branch in findAll(). Bonus: forcing a single mode is actually cheaper than today, since it skips the fallback lookup.

Issues with this approach:

1. Session currently does new ElementFinder(...) itself with no way to inject a different one (Session.php:46 on master, same story on 2-architecture-changes). So we'd need to open that up too, e.g. an optional ?ElementFinder $elementFinder param on Session's constructor — there's already a precedent for that with the optional ?SelectorsHandler $selectorsHandler param.
2. DocumentElement::hasContent() also goes through 'named' (find('named', array('content', $content))), and "content" matching is supposed to be a substring match. If the exact mode gets forced globally, hasContent() would suddenly require the whole document text to match exactly, which is almost certainly not what anyone wants. Worth checking all the internal 'named' call sites (TraversableElement, NodeElement::selectOption, DocumentElement::hasContent) before this ships, since a global switch hits all of them, not just the spot someone actually meant to make exact.

Due to later the global switch would break document content lookup, and the per-method switch would break MinkExtension Behat integration that has no way to specify selector name on any find* call.

[andriokha]

Thanks for your time!

The ElementFinder could be the right place for this setting, not Session.

That works for me! It seems to me there's not tons of difference in using session vs elementfinder. The element finder's instantiated from the session, so a session-level setting can easily be passed into the element finder. For me it's a bit more elegant to add it to the finder (keep everything as local as possible to what it affects) but I think you'd have to make the finder class public, and I guess ideally non-final. The only advantage of using the session I can see is that it's already public.

DocumentElement::hasContent() also goes through 'named'

Yup I mentioned that one earlier. It's an easy fix IIUC (use named_partial) and I can't find other examples that would be similarly affected.

Btw I didn't release there was a 2.x branch - and I notice it doesn't include v1.13.0 in its history - which branch should I be looking at (and working on if I make a PR)?

[aik099]

Ah, then we're all set:

1. add an optional mode parameter to a ElementFinder and it will then populate a new class property based on that parameter and decide how named selector is handled
2. add an optional parameter to a Session
   • either a mode parameter and forward it to created ElementFinder instance
   • or ElementFinder instance itself (what about @internal and @final?)
3. (could even be standalone PR) use named_partial selector instead of named on the DocumentElement::hasContent (optimization as well)
4. confirm that tests are passing and add new ones to confirm it works as expected
5. in your code when instantiating Session specify mode parameter (or provide ElementFinder instance with mode parameter)

As for the 2.x branch would be fine because the ElementFinder who decides and Session is giving it the parameter from an outside. The fact that in the 2.x branch NodeElement is using ElementFinder directly (instead of Session) doesn't break anything.

@stof, what bothers me in this whole idea is that ElementFinder is marked as @final and @internal and we're changing it. What do you think?