User tests: Successful: Unsuccessful:
Pull Request for Issue #24095 .
use lower() to check user email
see #24095
test@test.fr and Test@test.fr must be detected as the same email address even on db's like Postgresql
detected as the different email address even on db's like Postgresql
mysql < 5.7 don't support function based index ... iirc
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
Category | Libraries | ⇒ | Front End com_users Libraries |
Category | Libraries Front End com_users | ⇒ | Administration Front End com_users Libraries |
Category | Libraries Front End com_users Administration | ⇒ | Administration Front End com_users Installation Libraries |
Category | Libraries Front End com_users Administration Installation | ⇒ | Administration Front End com_users Installation Libraries Plugins |
Title |
|
Title |
|
Category | Libraries Front End com_users Administration Installation Plugins | ⇒ | Administration Front End com_users Libraries Plugins |
you can test with https://docs.joomla.org/Component_Patchtester_for_Testers
Hello,
Thanks for the patchTester,
I tested the patch and it's OK.
Could you merge this branch and build a new patch?
Category | Libraries Front End com_users Administration Plugins | ⇒ | Administration Front End com_users Postgresql SQL Installation Libraries Plugins |
Category | Libraries Front End com_users Administration Plugins Postgresql SQL Installation | ⇒ | Postgresql SQL Administration com_admin Front End com_users Installation Libraries Plugins |
@mdaoudi123 please mark as tested on https://issues.joomla.org/tracker/joomla-cms/24117
this should be carefully tested on mysql side too....and mssql also i'm afraid
Hello,
im going to test it into POSTGRES too,
can you tell me how to mark this post as tested?
Thanks
I have tested this item
Tested Successfully on Mysql and Postgres databases.
Thanks
Hello,
I have already tested this feature.
Could you please creat a new patch @alikon
Hello,
I have already tested this feature.
Could you please creat a new patch @alikon
one test is not enough, we need at least two, before this pr can be considered by the maintaners
Labels |
Removed:
J3 Issue
|
sorry but really don't know how to fix CS for an inline function
I just wanted to say that the local part of an email is case sensitive, it's ok for me to ignore this because it's common practice to handle email addresses case insensitive.
@HLeithner @alikon
please there is news for this fix?
When this patch will be available?
Please Help
please could you fix the phpcsfixer warning
Any one can help me please
one test is not enough, we need at least two, before this pr can be considered by the maintaners
I tedted the fix. but who can test it another time to deblock the situation
Anyone can test please
Im waiting since 3 months
@richard67 you use Postgresql? Can you please look at this PR?
@franz-wohlkoenig Today I have a full schedule. I will try to find a time slot, but it might be tomorrow until I can do something. I do not really use postgresql, I just have it for testing.
@alikon @franz-wohlkoenig This PR will not work as it is. In the schema update, the index is dropped and then added back with same name. This is valid SQL, but the database schema checker can't handle that due to its bad design, so it will end in a toggle of reported database error "index missing but should exist" and "index existing but it should not". The only thing you can do is to add it back with other name, but the for consistency it might be necessary to change index name for MySQL, too.
thanks @richard67
btw,i don't think we need to do something on mysql side
@alikon I've made a PR adding the missing function comment which has to be completed by you, and which fixes a few of the code style issues. But I still do not understand why it needs that call back function stuff. Why don't you just make a local copy of the emails array, lowercase it, and use that for the query? It would not need this inline function and not need this new function and so no need for new function comments and all codestyle could be fine. Can you explain me why it needs to be done in that way with the callback?
ouch i've to admit that i don't remember why i've done that, in that way maybe in that period i worked at the office "complicate simple thing"
@alikon I see another problem with the new unique index on postgresql (and later in 4.0-dev where we have MySQL >= 5.7 on MySQL, too, if we add the same index then):
When users do an update of their Joomla and have duplicate lowercase emails in their users table database, like "gaga@blabla.de" and "GaGa@blabla.de", the unique index adding will fail.
It would require to clean existing data by duplicate records. We once had the same problem when we wanted to change collations of com_finder tables in MySQL from general to unicode, because that change would have caused duplicate values, and so we gave that up and remained com_finder tables with general collations.
If you want to keep that new index for performance reasons, then make it non-unique, i.e. just an index.
Or you don't add that new index.
Of course this would not be a problem if we could be sure that the PHP code of the CMS never allowed to add users having same lowercase email address. But I am pretty sure even if that was the case, we could not 100% rely on it over all the history of Joomla version in the update history of a web site.
So I suggest to make the new index non-unique or leave it away.
@alikon P.S. to my previous post: I think @mdaoudi123 did not have that problem during his test because either he did not have duplicate records, or he did not run the schema update sql (which patchtester will also not do) and so he tested only the changes in PHP but not the database change.
thanks @richard67
Drone failure is not related to this PR, is javascript stuff. PHPCS is ok now.
@mdaoudi123 Sorry for the inconvenience and the long waiting time. Could you test this PR again with its latest changes?
Unless there are conflicts there is no benefit
Yes but looks less scary for beginner testers.
I have tested this item
Tested on MySQL and PostgreSQL.
On MySQL it does not break anything.
On PostgreSQL it checks now for present email address in case-insensitive way without touching existing user data.
Tested as follows:
Pre-conditions: In Global configuration, set error reporting to maximum or development and watch the PHP error logs during the tests. Check that there are no errors reported.
@alikon Could you update your testing instructions to what I wrote here above so other users can test more easily?
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-08-03 10:11:51 |
Closed_By | ⇒ | alikon |
Status | Closed | ⇒ | New |
Closed_Date | 2019-08-03 10:11:51 | ⇒ | |
Closed_By | alikon | ⇒ |
Status | New | ⇒ | Pending |
I have tested this item
@mdaoudi123 Sorry for the inconvenience.
Could you test this PR again with its latest changes?
And then mark your test result at the issue tracker here https://issues.joomla.org/tracker/joomla-cms/24117, using the "Test this" button at the top left corner? Then just select the appropriate result and submitt.
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC.
Thank's a lot.
it's up to the maintainers/release lead to decide if and when
Can you please rename the sql file to 3.9.15 then I will merge this.
Labels |
Added:
?
|
renamed
I'm not the biggest fan this PR but think it solves more problems then it creates.
Thanks
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-01-08 13:13:58 |
Closed_By | ⇒ | HLeithner |
Thanks a lot for your help.
But There is also another file where exists a email comparaison:
and into other files..... (search for "$db->quoteName('email')")
Could you please add the correction to thoses files @alikon