Allow the named selector to skip the partial fallback

[Amoifr]

Fixes #880.

Implements the shape @aik099 settled on: an optional mode on ElementFinder deciding how the named selector resolves, and an optional mode on Session that forwards it. Nothing outside those two constructors changes, and both default to today's behaviour.

// exact matches only, the way "named_exact" behaves
$session = new Session($driver, $selectorsHandler, ElementFinder::NAMED_EXACT);

ElementFinder stays untouched from the outside, which is the point of passing a mode rather than a finder instance: users never name a class that is marked @internal.

On the tests you asked for

The mode reaching ElementFinder is checked through behaviour rather than through a getter, since a fallback costs an extra driver query and that is observable. SessionTest covers both directions:

• testNamedModeIsForwardedToTheElementFinder builds a Session with NAMED_EXACT and asserts the driver is queried once, so no partial lookup happened.
• testTheDefaultNamedModeStillFallsBackToPartial builds one without the argument and asserts the driver is queried twice.

ElementFinderTest covers each mode symmetrically, two tests per mode: testNamedFound and testNamedPartialFallback for the default, testNamedExactModeStillReturnsExactMatches and testNamedExactModeDoesNotFallBackToPartial for the strict one. I checked they fail against an unmodified src/.

One design note

The condition reads self::NAMED_EXACT !== $this->namedMode rather than testing for the default. It is the safer way round: a mode that is neither constant falls back to the historical behaviour instead of silently switching to strict.

I first added a runtime guard rejecting unknown modes, then dropped it. PHPStan flagged the test for it, because the @param self::NAMED_* annotation makes an invalid argument impossible statically, and this repository keeps level 8 clean without a single inline @phpstan-ignore. Inverting the condition made the guard unnecessary rather than merely unenforced.

Full suite green, PHPStan clean. Point 3 of your plan shipped separately in #881.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.46%. Comparing base (9b08f62) to head (bb330a7).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/Selector/NamedSelectorMode.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #882      +/-   ##
============================================
- Coverage     98.56%   98.46%   -0.11%     
- Complexity      389      393       +4     
============================================
  Files            24       25       +1     
  Lines           909      913       +4     
============================================
+ Hits            896      899       +3     
- Misses           13       14       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[andriokha]

Hi @Amoifr, thanks for working on this, I'm super keen for it to get in! I had a couple of points of feedback:

• If ElementFinder is internal, then so are the constants (even public) exposed on it. If we're asking end users to pass the constants to the Session constructor, I think they should be on a non-internal class (Session in this case).
• As we can't use enums in PHP 7.2 and we're using constants in their place, I'd suggest validating $namedMode and throwing an InvalidArgumentException if it's not valid.

[stof]

I think we should rather put those constants on a dedicated class than on Session (if we were able to use a true enum, we would also have to name it)

[aik099]

Please:

1. rename this test into testNamedExactModeDoesNotFallBackToPartial
2. drop the testNamedExactModeDoesNotFallBackToPartial (declared above this one)

---

Most of test in this class assert search-behavior and return-behavior in the same method. No need to overcomplicate this by checking both things in separate test methods.

[aik099]

Maybe indentation is wrong (missing space before falling word).

[aik099]

Please use ::class with corresponding class import instead of specifying it's FQCN as string.

[aik099]

Agree with moving new class constants into a Session class and it would work fine in the Mink 2.x branch.

[Amoifr]

Thanks all three of you. Pushed everything that is not the open question.

@andriokha you are right, and it undercuts my own argument on the issue: I said @internal means users never name the class, then asked them to write ElementFinder::NAMED_EXACT. The constants have to live somewhere public. Where is now the only thing left, and @stof and @aik099 do not agree:

@stof: I think we should rather put those constants on a dedicated class than on Session
@aik099: Agree with moving new class constants into a Session class

Say which and I will move them in the next push. If it is a dedicated class it also needs a name, which is @stof's point about naming the enum we cannot write.

The validation is back, @andriokha, with one consequence worth stating rather than hiding. The narrow @param self::NAMED_* and a runtime guard cannot coexist here: PHPStan then rejects the very test that passes an invalid value, and this repository keeps level 8 clean without a single inline @phpstan-ignore. I picked the guard and widened the annotation to string, because the guard protects everyone at runtime while the annotation only helps callers who run a static analyser. Tell me if you would rather have it the other way and I will swap them.

@aik099, the three review points:

• the @param continuation was one space short, exactly as you saw. Fixed.
• ::class with the import, instead of the FQCN string. I left the two in prepareSession() alone since they predate this pull request.
• the two exact-mode tests are now one, under the name you asked for. I kept the empty-result case rather than the matching one: it is the only case where the two modes behave differently, since a successful exact lookup never falls back in either mode, and testNamedFound already covers that path. It asserts the single driver call and the returned value together, the way the rest of the class does.

Suite green at 526 tests, PHPStan clean.

[stof]

The narrow @param self::NAMED_* and a runtime guard cannot coexist here: PHPStan then rejects the very test that passes an invalid value, and this repository keeps level 8 clean without a single inline @phpstan-ignore. I picked the guard and widened the annotation to string, because the guard protects everyone at runtime while the annotation only helps callers who run a static analyser. Tell me if you would rather have it the other way and I will swap them.

Actually, we can have the precise type in phpdoc and the runtime guard by setting treatPhpDocTypesAsCertain: false in the phpstan config.

[stof]

I suggest going for the separate class to hold the constants.

Based on the packagist installation stats, I'm considering dropping support for PHP <8.1, which would then allow turning this separate class into an actual enum before releasing it.

[Amoifr]

Done, @stof. The constants now live in Behat\Mink\Selector\NamedSelectorMode, and neither ElementFinder nor Session declares any.

I put it in the Selector namespace rather than next to ElementFinder, because it sits beside the two things it chooses between, ExactNamedSelector and PartialNamedSelector. The name reads the same way once it becomes a real enum, NamedSelectorMode::EXACT, so the eventual conversion should be a rename of the keyword and nothing else. The class is final with a private constructor in the meantime, so nobody starts instantiating it.

use Behat\Mink\Selector\NamedSelectorMode;

$session = new Session($driver, $selectorsHandler, NamedSelectorMode::EXACT);

That also answers @andriokha's objection properly: nothing users have to name is marked @internal any more.

Suite green at 526 tests, PHPStan clean.

[andriokha]

Suggested change

	*
	* This is the historical behaviour, and stays the default.

I'd suggest avoiding describing historical behavior - code comments should describe how things work now imho. Source control can give historical information.

[andriokha]

I think the advantage of switching off treatPhpDocTypesAsCertain is we can be more specific with the type here without upsetting phpstan.

Suggested change

	* @param string $namedMode How the "named" selector is resolved: one of the
	* @param NamedSelectorMode::* $namedMode How the "named" selector is resolved: one of the

[Amoifr]

Both applied, thanks @andriokha. The second one corrects something I got wrong earlier in this pull request.

I claimed the narrow @param and the runtime guard could not coexist here. They can. With your suggestion the annotation is NamedSelectorMode::* on both Session and ElementFinder, the guard stays, its test stays, and PHPStan reports no error. I checked that PHPStan is genuinely still looking rather than silently skipping the file: passing an int at that same call site is reported straight away, and removing it goes back to clean. So callers get the static narrowing and everyone gets the runtime check, instead of the trade I described.

The historical note is gone from the constant, you are right that git carries that.

One thing I have left as is, so you can tell me if you would rather have it otherwise: codecov/patch is at 87.5% because of the private constructor on NamedSelectorMode, which by design can never be called. It is what makes the class a faithful placeholder for the enum @stof has in mind, since an enum cannot be instantiated either. I would rather keep it and live with the number than drop it to make the check green, but it is a one-line change if you disagree.

Suite green at 526 tests, PHPStan clean.

[andriokha]

One thing I have left as is, so you can tell me if you would rather have it otherwise

Just for transparency, I'm a nobody :)

I would rather keep it and live with the number than drop it to make the check green

Dunno if the maintainers think it's worth it, but you can always do something like the following prior to attempting to instantiate the class:

$this->expectException(\Error::class);