search

php / web-pecl

The PECL website
http://pecl.php.net
Other
31 stars 31 forks source link

[WIP] Fix administration login issues #70

Closed petk closed 5 years ago

petk commented 5 years ago

This fixes admin login and some other fixes on an obsolete Karma class until database schema migration will be done...

salathe commented 5 years ago

Which change(s) actually fixes the admin login problem? I'm struggling to see it in amongst the seemingly unrelated other changes.

petk commented 5 years ago

The PDO returns string for the admin field from the database and PHP version currently installed in production. On current PHP versions it works differently. Other changes are done in this context to omit several objections so far to not do change for the sake of changes. And because there is planned more here...

salathe commented 5 years ago

The PDO returns string for the admin field from the database and PHP version currently installed in production. On current PHP versions it works differently.

Okay, thanks for the explanation. A PR with only changes addressing that problem would have been better.

Other changes are done in this context to omit several objections so far to not do change for the sake of changes. And because there is planned more here...

Change for the sake of change is not wanted at all. It doesn't make it any better to include those changes in amongst meaningful changes.

Please try to keep the commits focused on a single subject for a given PR, rather than mixing in unrelated changes. Not doing so only serves to make life more difficult for potential reviewers and by consequence more likely to have errors slip through the net.

petk commented 5 years ago

Considering what people understand with terms of change for the sake of change (not targeting only your comments but some others in other pull requests and mailing list comments) I'm not sure this app can move in any direction then. So, I'll see what can be done with this and still move it to where it should be...

salathe commented 5 years ago

Can you keep only the actual admin login fixes for this PR? I'm not against the other changes that were made, just not here.

petk commented 5 years ago

Sure, sending separate commits...