Skip to content

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Nov 28, 2017

Factory::getApplication(); doesn't take any arguments anymore and will return always the global application object which must be set by the front controller during the set up phase. The problem on that part in J3 is that the code

$app = JFactory::getApplication('site');
$app = JFactory::getApplication('administrator');
echo $app->getName();
// Returns site and not administrator

will always return the site app, despite the administrator app is requested. This can be done now trough Factory::getContainer()->get('SiteApplication'); which will return a site application even on the back end.

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

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2018

So if we do this do we actually want to have the CMSApplication::getInstance method at all? Because that's going to be totally unused. Thoughts @mbabker

@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2018

If we are removing the app singletons we need to ensure that running the app constructors multiple times doesn't create side effects on the primary app and dependencies.

@laoneo
Copy link
Member Author

laoneo commented Feb 2, 2018

CMSApplication::getInstance is deprecated anyway in favor of getting them from the container. If we need singletons, then we can make them shared in the container?

@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2018

Most of the container services are shared already anyway. But even outside of that, my point isn't about having singletons but about ensuring that running new SiteApplication multiple times doesn't cause issues because there is some arbitrary logic in the constructors that can affect global services like the session.

@laoneo
Copy link
Member Author

laoneo commented Feb 2, 2018

I guess we should keep CMSApplication::getInstance for BC during the 4 series. This would avoid then having multiple applications or?

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2018

Ahh i missed CMSApplication::getInstance was checking in the container. Sorry. Then I guess this is fine

@wilsonge wilsonge merged commit 58e1956 into joomla:4.0-dev Feb 2, 2018
@wilsonge wilsonge deleted the j4/factory/application-parameters branch February 2, 2018 16:22
@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 2, 2018
@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2018

Regardless of whether the getInstance method lives forever or disappears tomorrow, we still need to ensure that running the constructors multiple times doesn't create side effects. That is the only concern with removing this specific singleton store, every other service that has singleton doesn't have global reach like the application does.

  • These lines in the CMSApplication constructor which changes the configuration regarding some session data
  • The loadSystemUris method in the WebApplication which is parsing the request URI and storing data in the configuration
  • This line in the AdministratorApplication constructor which is resetting Joomla\CMS\Uri\Uri::root()

This is a move in the right direction, just pointing out technical concerns in getting to the end state.

@laoneo
Copy link
Member Author

laoneo commented Feb 5, 2018

Needs the "Documentation Required" label.

@wilsonge
Copy link
Contributor

wilsonge commented Feb 5, 2018

Thanks done!

@mbabker just to confirm that only applies to when we remove CMSApplication::getInstance and not this PR right?

@mbabker
Copy link
Contributor

mbabker commented Feb 5, 2018

Right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants