User tests: Successful: Unsuccessful:
This PR does not change the system, but optimizes it in a few key areas. This helps with testing and removes some minor potential issues.
The initialisation is done on onAfterInitialise instead of on constructing the plugin. That saves the additional code to call the parent constructor.
Since there is only one string in this plugin, the autoload feature is disabled and the language file is only loaded right before the string is used.
All calls to the application object or the config object have been replaced by calls to $this->app, which is autoinjected into the plugin. This allows to write unittests for this with a fake application object.
Reading and setting the cookie is done through the input object and not with setCookie. Also, the path for the cookie is set to the root of the Joomla installation. This prevents interferences between different installations on the same host. If you have for example a Joomla in /joomla and cloned version of that installation in /joomla_test, the cookie would be set when you logout of /joomla and /joomla_test would read that cookie and thus register the error handler. The chances are slim, but anyway...
Labels |
Added:
?
|
There are 4 places where we set a cookie with setCookie() in Joomla, the session, in com_banners, the logout and the languagefilter plugin. The logout plugin would be done with this one, the languagefilter plugin is something that is a WIP and I wouldn't change the session. That leaves com_banners and I'm happy to provide a patch for that. Yes, you can set the path in the configuration, but that still requires you to actually touch that part and remember to set it there. This solution would be the better one.
The other issues should be fixed.
Are you saying that with this code the option to set the cookie in the
configuration is not used? If that is so then what happens to the existing
sites that have it set.
On 4 February 2015 at 11:34, Hannes Papenberg notifications@github.com
wrote:
There are 4 places where we set a cookie with setCookie() in Joomla, the
session, in com_banners, the logout and the languagefilter plugin. The
logout plugin would be done with this one, the languagefilter plugin is
something that is a WIP and I wouldn't change the session. That leaves
com_banners and I'm happy to provide a patch for that. Yes, you can set the
path in the configuration, but that still requires you to actually touch
that part and remember to set it there. This solution would be the better
one.The other issues should be fixed.
—
Reply to this email directly or view it on GitHub
#5964 (comment).
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
No, I'm saying that it uses the current root of the Joomla site as a default value instead of simply /. If the value is set in global configuration, that one overrules this default setting.
Thanks for clarifying that part
On 4 February 2015 at 11:41, Hannes Papenberg notifications@github.com
wrote:
No, I'm saying that it uses the current root of the Joomla site as a
default value instead of simply /. If the value is set in global
configuration, that one overrules this default setting.—
Reply to this email directly or view it on GitHub
#5964 (comment).
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
There are 4 places where we set a cookie
Actually, six places: https://github.com/joomla/joomla-cms/search?utf8=%E2%9C%93&q=cookie_path&type=Code where cookie_path
is used.
You forgot the remember me plugin.
The sixt one is a deprecated helper function.
I just want to have it consistent in all places where it is used. And that is best assured if done in one PR which only deals with that issue.
On the other hand, I'm not sure if it's needed. The default value we have works for most sites and is simple, fast and predictable. No need to use a static method, just straight a single character.
But I don't care much about how it is, as long as it is consistent in all places.
Then set it in the JInputCookie class to the one value and remove all other cases. Unless the value is set specificly to one thing, the cookie_path from configuration is used. If that is not available, use JURI::current() or '/', I don't care. That would actually save us a lot of unnecessary code. At the same time, it couples JInput and JApplication.
Category | ⇒ | Plugins |
Title |
|
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-01-06 11:31:15 |
Closed_By | ⇒ | Hackwar |
@Hackwar Hannes, please test your PRs before submitting...
$uri
isn't defined and creates a fatal error.If you want to change the default value for
cookie_path
from/
to$uri->base(true)
I would rather see it in an own PR, but then done for all instances. Afaik we have that default value in all place where the cookie_path is retrieved.But then, this is why we can specify the path in the configuration.