? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
2 Feb 2018

Ongoing effort to phase out getInstance code. This pr deprecates Pathway::getInstance.

@wilsonge and @mbabker can the code which gets the pathway from the factory being removed here?

I'm splitting #16918 into different pr's to be easier to review.

avatar laoneo laoneo - open - 2 Feb 2018
avatar laoneo laoneo - change - 2 Feb 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 Feb 2018
Category Libraries
avatar wilsonge
wilsonge - comment - 5 Feb 2018

I'm unsure about this - the pathway is constant for a given page - we shouldn't load this multiple times. If we aren't doing a getInstance it at least needs to be in the container and retrieved consistently through the container. We shouldn't be recommending creating a fresh instance each time

avatar laoneo
laoneo - comment - 5 Feb 2018

True, was a bit in a rush and didn't saw that it caches the instances per client. Sounds like another factory in the container. Do we want to go that way?

avatar wilsonge
wilsonge - comment - 5 Feb 2018

I think just do it like the application in the container yeah? This ones kinda weird because there isn't an Admin Pathway (at least at the moment) - although with the admin menu changes in 3.8 it wouldn't totally surprise me if one came along at some point

avatar laoneo laoneo - change - 5 Feb 2018
Labels Added: ?
avatar laoneo
laoneo - comment - 5 Feb 2018

Modified the pr to get the pathway object from the container by a service provider. Changed the deprecate message to get the pathway from the app, as every core extension does already.

avatar mbabker
mbabker - comment - 7 Feb 2018

A little late to the party here, but...

Shouldn't there just be a $this->pathway property on the application object? If it's supposed to be persistent for the lifetime of the request, the pathway object should be very closely bound to the active application and should live as a property there instead of always being reliant on fetching the object out of the container (we can still use the container to build the initial service, and we should drop the unused options param from the constructor as well).

avatar laoneo
laoneo - comment - 7 Feb 2018

Added your input.

avatar wilsonge wilsonge - change - 11 Feb 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-02-11 12:13:40
Closed_By wilsonge
avatar wilsonge wilsonge - close - 11 Feb 2018
avatar wilsonge wilsonge - merge - 11 Feb 2018

Add a Comment

Login with GitHub to post a comment