MNT Handle LogicExceptions when calling setI18nLocale()

[emteknetnz]

Issue silverstripe/.github#357

Fixes https://github.com/silverstripe/silverstripe-behat-extension/actions/runs/12938868907/job/36089987699?pr=295#step:12:101

1) SilverStripe\BehatExtension\Tests\SilverStripeContextTest::testGetRegionObjThrowsExceptionOnUnknownSelector LogicException: getItemPath returned null for SilverStripe\BehatExtension\Tests\SilverStripeContextTest.

SilverStripeContextTest does some weird stuff with mocking. In these sorts of scenarios I'm assuming the type of unit testing that we're running won't be reliant upon having to set the en_US locale in order to get the test to always pass.

Kitchen sink CI run with this PR - https://github.com/creative-commoners/recipe-kitchen-sink/actions/runs/12979001917

[GuySartorelli]

Seems like SilverStripeContextTest is the only situation we expect this to fail - I'd be much more comfortable with a if ($this instanceof SilverStripeContextTest) { return; } at the top of the method.

[emteknetnz]

Updated. I've used a string classname rather than importing SilverStripeContextTest and class_exists() and SilverStripeContextTest::class syntax because behat-extension in not a required module, and nor should it be IMO

[GuySartorelli]

Use a use statement - mostly because when searching for FQCN (e.g. if the namespace changes) we're likely to put SilverStripe\BehatExtension\Tests\SilverStripeContextTest in the IDE, not double \\

[GuySartorelli]

Note that a use statement doesn't require that the class exists. It just is a reference for the IDE to know that when it sees the used name, it should expand to the FQCN.

[emteknetnz]

I've changed it to single \ rather than double \\ PHP doesn't mind which way you do it, I just prefer the double

I'm not going to import the class as what this PR is already doing is bad enough, importing a subclass from an optional module into the base class is just gross

[GuySartorelli]

We should discuss this in person to get us on the same page about it, because to me a hardcoded string is the gross thing.

[GuySartorelli]

For reference, there's already precedent even in our own code for using a use statement for classes that aren't guaranteed to exist:

• https://github.com/silverstripe/silverstripe-spamprotection/blob/2ee91dda5e38f9219e64f45437bc541ca6536606/src/EditableSpamProtectionField.php#L19
• https://github.com/silverstripe/silverstripe-taxonomy/blob/9834c0336f7096604b9ada1c72c46e957b05b9d4/src/Controllers/TaxonomyDirectoryController.php#L12
• https://github.com/silverstripe/silverstripe-session-manager/blob/120b612dcad6692bfe638e44b3261afd390c3e33/src/Jobs/GarbageCollectionJob.php#L15
• https://github.com/silverstripe/silverstripe-fulltextsearch/blob/7609ca8716654f0ca64ffdbb41f93b38c3a4997a/src/Search/Captures/SearchManipulateCapture_PostgreSQLDatabase.php#L9
• https://github.com/silverstripe/silverstripe-versioned-snapshots/blob/918b0548ce249cc14991e702a5aeafd9315271aa/src/Migration/Job.php#L9
• https://github.com/silverstripe/silverstripe-blog/blob/dbd9d7921fd51459ecebae912a06d5c56339d2ed/src/Widgets/BlogArchiveWidgetController.php#L8

Any more. Granted these are all class_exists() checks - if you want to include a class_exists() in the condition e.g change the condition to if (class_exists(SilverStripeContextTest::class) && ($this instanceof SilverStripeContextTest)) then that's totally fine - but ultimately I think there's no good reason to hardcode a FQCN when the explicit purpose of use is to avoid that.

[emteknetnz]

importing a subclass from an optional module into the base class is just gross

IMO we just shouldn't do this. it's pure code smell. The string FQCN is less offensive, though I still don't actually like it.

Those examples you've provide are a different scenario where they are all optional modules (unlike framework, which is the base module), and the classes with the class_exists() checks are all classes that becomes available on the condition of another optional module (a sibling module) being installed

Non gross solution here is to revert it back to how it was before and just catch a LogicException

OR, if you're concered that's too blunt

Add some new protected API into SapphireTest protected bool $doUpdateI8nLocale = true (or something named similarly), set that to false in that behat-extension unit test, and then (maybe, not sure if needed) add a conflict in silverstripe/framework composer.json for silverstripe/behat-extensions so that the --prefer-lowest build doesn't break

Let me know what solution you'd like (excluding importing the FCQN :-) )

[GuySartorelli]

If by "importing the FQCN" you mean using a use statement, then we need to discuss in person because that's the solution I'd like.

[GuySartorelli]

But we'll never agree on that for some reason (I really really don't understand your perspective on this at all)..... so if we absolutely can't do that for some obscure reason, the protected property would be preferable.

[emteknetnz]

Have updated PR, and create additional PR for behat-extension

[GuySartorelli]

LGTM