Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Use the options from the app instead of the global config #19531

Closed

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Feb 2, 2018

Use the config variables in the session service provider from the application. Question here, should the app not being fetched from the container instead of the Factory?

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

@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2018

Question here, should the app not being fetched from the container instead of the Factory?

Well, here's the problem. The session service is instantiated by the application service, so $container->get('application'); would end up in a recursive loop between that and the session. We also are not yet injecting a Registry instance into the application representing the parsed JConfig object, so we're still relying on things to inherently resolve themselves.

Eventually, we will need a config service in the container similar to the one defined in https://github.com/joomla/framework.joomla.org/blob/master/src/Service/ConfigurationProvider.php (the part about how the service is shared, not necessarily having the provider parse the config itself). This way when there are services which require the configuration (i.e. https://github.com/joomla/framework.joomla.org/blob/master/src/Service/DatabaseProvider.php) then that can be accessed without being dependent on a complex service like the application, especially when the dependency exists while the application dependency is being resolved.

@laoneo
Copy link
Member Author

laoneo commented Feb 3, 2018

Made a quick test and I could move setting the session from the app service provider here to the execute function. So it should be possible to load the session after the app is created in the application service provider. As said just a quick test didn't test other stuff.

@mbabker
Copy link
Contributor

mbabker commented Feb 3, 2018

Well, you can do that, but then you've got the application arbitrarily injecting its own dependencies (either by creating them inline or accessing the container). So we're losing a huge piece of testability here if $app->execute() is doing $this->setSession($container->get('session')); without mocking the container, not much better than what we have now with putting stuff into JFactory's public statics.

This is just one of those spots that aren't pretty because configuring the session service is reliant on the active application. So either the session config keeps a dependency to JFactory or you break dependency injection on the application.

@laoneo
Copy link
Member Author

laoneo commented Feb 5, 2018

You got me wrong. I didn't say I want to do it that way. Just wanted to test if the session instance is really needed when creating the application in the container.

@mbabker
Copy link
Contributor

mbabker commented Feb 5, 2018

To create the application, the session isn't needed. But to create the session, the application is needed. That's where we are running into problems.

@mbabker
Copy link
Contributor

mbabker commented Feb 5, 2018

Just to be clear here, it's not an architectural dependency; it's all configuration based (the force SSL option being on a per-client basis as an example, or the default value of the session_name config option being the class name of the primary application for the request, the one which should be in the factory).

@laoneo
Copy link
Member Author

laoneo commented Feb 12, 2018

I guess we shifted here. Lets get the session issue sorted in another pr then.

…der/session/config

# Conflicts:
#	libraries/src/Service/Provider/Session.php
@laoneo
Copy link
Member Author

laoneo commented Feb 12, 2018

Conflicts fixed.

@mbabker
Copy link
Contributor

mbabker commented Feb 12, 2018

Sooner or later we're really going to need a $container->get('config'); that gets injected into apps versus relying on the apps using the Autoconfigurable behavior (Factory::getConfig() can proxy to that for the time being so the same root config registry is still accessible).

For now this PR seems fine, but that really needs to be one of the next architecture steps we take.

@laoneo
Copy link
Member Author

laoneo commented Feb 12, 2018

I will have a look into the configuration service provider.

@laoneo
Copy link
Member Author

laoneo commented Feb 13, 2018

Pr for config service provider #19658.

@laoneo
Copy link
Member Author

laoneo commented Feb 13, 2018

Closing this one then.

@laoneo laoneo closed this Feb 13, 2018
@laoneo laoneo deleted the j4/provider/session/config branch February 13, 2018 07:27
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.

None yet

3 participants