joomla
[5.1] com_users: Simplifying empty check in login layout
Summary of Changes
I fixed bugs in an old layout override and stumbled over this. This is a very complex way of checking if there is anything in that parameter. We could make it worse by using regex and maybe doing an !empty() check around all of this... Anyway, this check just tries to find out if the description parameter does contain anything of relevance. Relevant is everything that remains after removing spaces from beginning and end of the string, thus a trim() is enough.
Testing Instructions
Codereview
Link to documentations
Please select:
-
[ ] Documentation link for docs.joomla.org:
-
[X] No documentation changes for docs.joomla.org needed
-
[ ] Pull Request link for manual.joomla.org:
-
[X] No documentation changes for manual.joomla.org needed
if you look in the history you will see that it was deliberately changed from what you are proposing to what it is now
The PR in question is #9896. Even looking at the PR, I can't understand why this was merged. Regardless of that, the browser in question is EOL and not supported by us anymore. I still think that this PR is the right way to go.
if you look in the history you will see that it was deliberately changed from what you are proposing to what it is now
When reading that old PR‘s code changes I can’t see that it had any trim before. It was comparing the complete unmodified string. So this PR here is not a roll back of the changes from the other PR.
@Hackwar If you want to make it work exactly the same way as the old code regarding other kinds of white space than a single space character, you could use a string with a single space as 2nd parameter for trim, then it would trim only space characters. Not sure if it needs that, but we would play safe and it still has all advantages of your code change.
interesting your change changes the behavior more then I expected:
https://3v4l.org/7LM6I (yours is $z)
This is why it is always important to look at why code was written the way that it was before changing it
Yes, this "changes" behavior. It actually fixes it to be in line with what people expect. The goal is to only show the text when it contains anything. Right now it only removes spaces, but would keep a line break or tab and then show the text. Yes, it also hides it, when the content is only a 0, but I would also argue, that there is no situation where someone would want a single 0 as description. The goal of this PR is to simplify the code and make it readable for people. We want to remove invisible characters of all kinds, so I'm not going to limit the functionality of trim() to just remove spaces. If that is not acceptable, then rather lets close this PR than to waste more time on this.
Seeing the logic comes back with the summary. However, it is not possible for me to perform a test. Seeing the documentation of PHP trim function, it cleans up all types of blank spaces in a string.
I have tested this item :white_check_mark: successfully on 7ca9182a8ddc2563b7668fb6aa018e0c04869bcf
I have read the code proposed in the PR, and I have checked the PHP documentation (https://www.php.net/manual/en/function.trim.php). The PR certainly brings an advantage: it recognizes and removes not only empty spaces but also other hidden special characters.
" " (ASCII 32 (0x20)), an ordinary space. "\t" (ASCII 9 (0x09)), a tab. "\n" (ASCII 10 (0x0A)), a new line (line feed). "\r" (ASCII 13 (0x0D)), a carriage return. "\0" (ASCII 0 (0x00)), the NUL-byte. "\v" (ASCII 11 (0x0B)), a vertical tab.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41677.
I have tested this item :white_check_mark: successfully on 7ca9182a8ddc2563b7668fb6aa018e0c04869bcf
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41677.
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/41677.
