? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
4 Feb 2015

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...

avatar Hackwar Hackwar - open - 4 Feb 2015
avatar jissues-bot jissues-bot - change - 4 Feb 2015
Labels Added: ?
avatar Bakual
Bakual - comment - 4 Feb 2015

@Hackwar Hannes, please test your PRs before submitting...
$uri isn't defined and creates a fatal error.

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...

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.

avatar Hackwar
Hackwar - comment - 4 Feb 2015

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.

avatar brianteeman
brianteeman - comment - 4 Feb 2015

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/

avatar Hackwar
Hackwar - comment - 4 Feb 2015

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.

avatar brianteeman
brianteeman - comment - 4 Feb 2015

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/

avatar Bakual
Bakual - comment - 4 Feb 2015

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.

avatar Hackwar
Hackwar - comment - 4 Feb 2015

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.

avatar zero-24 zero-24 - change - 4 Feb 2015
Category Plugins
avatar vdespa vdespa - change - 14 Mar 2015
Title
Optimizing system logout plugin
[REFACTORING] Optimizing system logout plugin
avatar Hackwar Hackwar - change - 6 Jan 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-01-06 11:31:15
Closed_By Hackwar
avatar Hackwar Hackwar - close - 6 Jan 2016

Add a Comment

Login with GitHub to post a comment