[4.0] Fix pathway tests to match old behaviour

[wilsonge]

A slightly different approach to #19669 that stays a bit truer to the original 3.x code. @laoneo and @mbabker thoughts? I'm not so keen on the creating container for tests - I guess we should really inject a mock container and check it's has and get methods are called with the correct params. On the positive side much less Factory::getContainer(); stuff...

This also allows you to use the $name param because with the pathway var in the application we were only going to use whatever it's first value was. Now we can have multiple pathways (although quite what an empty string array key does idk but it doesn't break the tests so i guess it's ok??)

[mbabker]

Seems fair to me. I'd suggest checking against an empty string though so we don't hit that "who knows what's gonna happen" thing.

[laoneo]

Honestly I'm more in favor of having a pathway object per application than having the SiteApplication holding the pathway object for the Admin (I know this is not possible atm).

[wilsonge]

I understand but if we do that then we need to remove the name param outright because your code doesn’t stop the siteapp loading the admin pathway. But just means if that is loaded first then the site pathway is never loaded :/ I decided to stick closer to the original with the $name parameter than to try and muck around with it all

[laoneo]

IMO having a store of different pathway objects should not be done in the application. Having for that JPathway::getInstance is fine. But you are right, the name attribute is not needed anymore in the actual case.

[wilsonge]

Seems fair to me. I'd suggest checking against an empty string though so we don't hit that "who knows what's gonna happen" thing.

There is already a test - but I just don't like the concept so I've appending a hardcoded pathway string afterwards.

IMO having a store of different pathway objects should not be done in the application. Having for that JPathway::getInstance is fine. But you are right, the name attribute is not needed anymore in the actual case.

As long as we can get multiple applications out (I've not tried it yet - because there's might well be cases where we might want to get the SitePathway in the Admin)

[laoneo]

Just tested it in the article view in the back end. When you do

$app = Factory::getContainer()->get(SiteApplication::class);
$p = $app->getPathway();

$p is the SitePathway object. Loading the different applications trough the container does work on a first glance. If Michael can detach the session from the global application then we should be on the safe side.

[laoneo]

And I would love if we can prepare the core that it will be possible to work with different applications than the default one from the factory.

[wilsonge]

My point is if you do

$app = Factory::getContainer()->get(SiteApplication::class);
$p = $app->getPathway('');
$p2 = $app->getPathway();

You are going to end up with $p2 being a Pathway instance not a SitePathway instance

[laoneo]

That's why...

But you are right, the name attribute is not needed anymore in the actual case.

[wilsonge]

I'm not too fussed. I preferred to keep compatibility with the old way. I don't feel strongly tho. @mbabker want to make a decision as the "independent"

[mbabker]

For now because the name argument is there in the application and just acts as a proxy to the existing getInstance method, let's not do the hard B/C break on not-yet deprecated functionality and take this PR. To remove the name argument (which is IMO a good idea, you should have one pathway for the application instance you're working with), let's first deprecate that functionality in 3.x.

[laoneo]

Lets be honest here, we are talking about an artificial BC break as there is only one Pathway class anyway, the SitePathway.
But if you guys feel comfortable, let's merge it. I would like to have the unit tests working again.

[mbabker]

It's still a B/C break. I have no issue making it, but without at least deprecating the logic that seems pretty unfair even if it is a massive edge case.

[mbabker]

(FWIW I've done a lot of work in the Framework repos where I do make B/C breaks, but more often than not before that PR is merged the corresponding PR marking deprecations gets created and merged first, mainly because I don't know what's getting deprecated/replaced until I reach a feature complete state; so all I'm suggesting is to do the same here)

[laoneo]

Here we go #19700.

[laoneo]

And here it will be removed, #19703. Hope all is fine now.

[wilsonge]

Closing in favor of #19703.