@Joey3000 opened this issue on September 1st 2015

To the Piwik\Plugins\Login\Controller class: The current use of "private" prevents class variable and method inheritance in child classes. The "protected" ones on the other hand, offer the same access control, but can be inherited.

Impact: I'm working on a plugin to encrypt the transmitted password. That plugin "extends" many of the classes of the core Login plugin. The current usage of "private" forces me to, unnecessarily duplicate (copy-paste) the "private" members of the Login parent class, without any modification. I tested this patch with 2.14.3 and it works flawlessly with my (future) plugin.

Also, it maybe could help https://github.com/torosian/LoginRevokable/issues/2.

Thanks in advance.

@Joey3000 commented on September 1st 2015

Unfortunately, \plugins\UsersManager uses many "private" as well. Maybe that could be changed, too. I was going to also do the password encryption when transmitted on password change by the user themselves or superuser (for other users). But that would be a different PR.

@tsteur commented on September 1st 2015

@diosmosis I think you worked on Login module a couple of times in the last months. Is Login\Controller an API? If so we should merge it and also mark it as @api. I reckon it is needed to have this controller as API to implement custom login behaviour?

@Joey3000 commented on September 1st 2015

@tsteur It's not needed for me. :) The plugin is small and lean just using class extension.

@Joey3000 commented on September 1st 2015

@tsteur @diosmosis Guys, is the Travis build failure shown related to the change in this PR? If not, do you think this could be merged into the upcoming release? That would be great.

@diosmosis commented on September 1st 2015

Is Login\Controller an API?

Currently Piwik will use the controller in the plugin defined by StaticContainer::get('Piwik\Auth')->getName(). So to create a login plugin, you effectively have to extend or reimplement Piwik\Plugins\Login\Controller. Shouldn't have to ideally, will probably change in 3.0.

@tsteur commented on September 2nd 2015

Thx for the pull request!

@sgiehl commented on September 4th 2015

Well, guess that one broke existing login plugins extending the piwik login plugin and makes it impossible to provide them for older piwik versions.

@tsteur commented on September 4th 2015

Sorry, I had no idea it was used by plugins, it was not marked as API so I didn't even have a look (apart from LDAP plugin). 2.15.0 is not released yet is there a chance to make it work for both 2.15+ and pre 2.15? Probably it's hard right?

@tsteur commented on September 4th 2015

We could revert this change if needed

@sgiehl commented on September 4th 2015

Not sure if there are other plugins having problems with that. Guess it's not suitable to make it work for older versions aswell. But as my plugin already required Piwik 2.14 it shouldn't be a too big problem to increase it to 2.15

@tsteur commented on September 4th 2015

The merged PR does not really have many advantages apart from not having to specify the private variable again right? If there's no big advantage we could revert it as there are possibly other plugins having this problem as well

@tsteur commented on September 4th 2015

I'll revert just to be sure to not break more plugins, sorry @Joey3000 Are you having any problems by this revert? Can you still make your plugin work?

@Joey3000 commented on September 4th 2015

I guess the breakage is that methods in child classes cannot have stricter visibility than their counterparts in parent classes. I.e. if the parent class declares a member (a variable or method) "private", the child can have it "private", "protected" or "public". Whereas if the parent declares it "protected", the child can declare it "protected" or "public" only, not "private", because it would be stricter. ==> If the child declares the member "private" while parent has it "protected" - PHP shows an error, which is my guess at the "breakage" here.

2.15.0 is not released yet is there a chance to make it work for both 2.15+ and pre 2.15? Probably it's hard right?

I guess it shouldn't be hard - just make the affected plugins change "private" members to "protected" in Controller the same way as well. It will then work for both, pre 2.15 (because it will be less strict than the parent member) and 2.15+ (where it's the same). Additional bonus for the extending classes: Other classes can then easily extend them as well, so that a plugin can extend another one, if needed.

The merged PR does not really have many advantages apart from not having to specify the private variable again right? If there's no big advantage we could revert it as there are possibly other plugins having this problem as well

With this PR: The extending classes (i.e. the login plugins) can now decide if they truly need to re-implement a Controller member (in which case set it to "protected"), or if it's just a copy-paste, whose only reason for existence was the fact that the parent had the member declared as "private", which was thus not accessible to the child (in which case just delete it).

I'll revert just to be sure to not break more plugins, sorry @Joey3000 Are you having any problems by this revert? Can you still make your plugin work?

Of course I can. I would just need to unnecessarily copy-paste variables and whole functions from core Login to my plugin. And to take the functions over again if they change in future.

With this PR, my Controller class has only two members: login() resetPassword(). Both of which are true re-implementations, which after some juggling call the parent methods of the same name. Everything else in Controller is provided by the core Login. I will hardly have to even provide translations, because it is just based on the core Login and re-uses it completely.

Without this PR, I would additionally need to copy-paste all the variables AND whole functions declared "private" in the Controller of the core Login. I.e. all those changed in this PR.

@Joey3000 commented on September 4th 2015

P.S.: I've had my morning coffee by now, so feeling a lot more relaxed. :)

Of the four obviously Login related plugins in the Marketplace (LoginHttpAuth, LoginLdap, LoginRevokable and ShibbolethLogin), I found one affected: https://github.com/torosian/LoginRevokable/blob/master/Controller.php. There is already an issue submitted on the copy-paste there: https://github.com/torosian/LoginRevokable/issues/2.

But there could be non-obviously related ones too.

So, whichever is best for the community. You guys have more experience with that (and PHP as well, by the way).

@sgiehl commented on September 5th 2015

@Joey3000 The changes in that PR are not bad. And imho we should change it, as it simplifies reusing the login plugin. But we shouldn't break compatibility at that point. I would suggest to move that changes to 3.0.

@Joey3000 commented on September 6th 2015

@sgiehl Thanks for feedback! I agree. I've published the plugin by the way, copy-pasting for now :) https://github.com/Joey3000/piwik-LoginEncrypted/blob/32a28b76355edb90b5bb2c9a414f174041d75895/Controller.php#L29 But the copies are already set to "protected", so won't break on this change in future.

@tsteur commented on September 7th 2015

@Joey3000 do you mind issuing another PR and to select 3.0 branch as target instead of master? It would be really great to have these changes. We don't want developers to have copy/paste such code. For 2.X we don't expect many changes in this area

@Joey3000 commented on September 7th 2015

@tsteur Done, thanks!

This issue was closed on September 2nd 2015
Powered by GitHub Issue Mirror