Before I comment about the code, here is one test I did, that had a problem:
1. log in as an unprivileged user
2. click on Change password
3. the panel opens, I enter a new password twice and click on Go
4. the message Loading appears and the panel stays there
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
In general, I find your refactoring quite good. Suggestions for now:
- put all functions at the end of script
- abbreviate less, or at least the same way (instead of Pass use Password)
- improve naming of functions that do more than one thing, for example PMA_ChangePassUrlParams()
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Thilina,
examples of improvement:
- Message "The profile has been updated." is twice in a very short span of code
- $is_error should have a more neutral name because it's not always for an error
- no need for $no_password because it only depends on one request variable; so verify directly the request variable inside the function
- variable $succesMessage should be renamed, for example $successMessage
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Refactoring for user_password.php file
Before I comment about the code, here is one test I did, that had a problem:
1. log in as an unprivileged user
2. click on Change password
3. the panel opens, I enter a new password twice and click on Go
4. the message Loading appears and the panel stays there
Ok Marc, May be I missed that. I'll look at that error. while I'm reproducing the patch can you give me some feedback about previous refactoring.
In general, I find your refactoring quite good. Suggestions for now:
- put all functions at the end of script
- abbreviate less, or at least the same way (instead of Pass use Password)
- improve naming of functions that do more than one thing, for example PMA_ChangePassUrlParams()
Hi Marc,
Thank you very much for your feedback and suggestions, Next time I'll definitely follow these.
Thilina,
can I expect another patch with at least the bug fixed?
Marc,
I'm trying to fixed that bug.
attempt 2
I have updated the patch.
The bug about the Loading message is fixed.
I found another one: when the user clicks on "No password", the message about password being empty should not be displayed.
attempt3
Hi Marc,
I have updated the patch file.
Hi Thilina,
The bugs are fixed. I see some improvements you could do; do you see them?
If not, I plan to merge your patch to origin/master (with yourself as the author) and do the improvements myself.
Hi Marc,
I'm looking my code, and try to find improvements, But still I couldn't found one, Is there any improvements in main if condition?
Hi Thilina,
examples of improvement:
- Message "The profile has been updated." is twice in a very short span of code
- $is_error should have a more neutral name because it's not always for an error
- no need for $no_password because it only depends on one request variable; so verify directly the request variable inside the function
- variable $succesMessage should be renamed, for example $successMessage
Thanks Marc, I'll do these improvements.
attempt4
Hi Marc,
I have updated the patch. I have implemented what you mentioned earlier.
IIRC, some time ago we decided not to use the @uses tags in function comments
Also, PMA_changePassDisplayPage() is missing the @param tags and all other @param tags are missing comments
Hi roccivic,
Do I need to remove @uses tags, I found that in [0]
[0] - https://github.com/phpmyadmin/phpmyadmin/commit/1394cc65e8bb3b5218ffb83dd3cb8490498bfe80
attempt5
Hi,
I have updated the patch by removing the @uses tags.
Thilina,
you refer to an old commit, see https://github.com/phpmyadmin/phpmyadmin/commit/e57f27e50ba565be52ed3d271d6c9076f2ae48fd