[#12627] - Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by)
- Fixed in Code Base
- 8 Dec 2016
- Medium
- Build: staging
- # 12627
- Diff
- matrikular:patch-15
- Success continuous-integration/drone the build was successful Details
- Error continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error Details
User tests: Successful: Unsuccessful:
Summary of Changes
Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by)
Additional changes:
Layout (Mootools Version ~ hathor):
- Change htmlspecialchars($VARNAME, ENT_COMPAT, 'UTF-8') to $this->escape($VARNAME)
- Refactor code to remove the ternary overuse
- Change JHtml::script call to JHtml::_('script' ...) to allow overrides via custom register method
- Correct variable name ($exclude) in doc block to match extracted name ($excluded)
- Remove obsolete extracted variables that are not present for this field type
Layout (jQuery / Bootstrap Version ~ isis):
- Same as for the Mootools implementation + removed the class suffix from the hidden field
- Remove (Fix) obsolete whitespace in class attribute for the visible field when no suffix was entered
JFormFieldUser:
- Satisfy Code Sniffer (e.g. @since)
- Remove else block after variable initialisation
- Fixe typo in doc block of the added custom fields handling method
- Changed getExcluded method return value to match doc block
Testing Instructions
- Open a new banner'' item in the back-end and switch to the "Publishing" tab.
Before the patch, the "Modified By" field shows a "Select a User" placeholder. Since you cannot really select a user here, that text is obsolete in that context.
''You could also open a new article but in the hathor template, the modified_by field won't show up until the article has actually been modified which would then of course override the placeholder. Hence, a banner item.
- Apply the patch, refresh the view or open another banner item. The placeholder should be gone.
Now, this PR changes both the "old" Mootools and the jQuery / Bootstrap implementation of the layout(s), where Mootools version is currently the "default". I put default in quotes because we think of isis as the default admin template and that template has got an override for the field layout which uses jQuery / Bootstrap code to open the modal window.
So, to fully verifiy that everything works as expected, please make sure that you test the field both in hathor and isis. That is, not only confirm that the placeholder text disappears from the "Modified By" field, but also that the "Created By" still works as it should (because it is the same field).
Caveat
I would go so far and move a few more lines, but before that, I would like your opinion first.
- Even though JHtml::script makes sure that a script only gets included ones, I would move the call for the modal stuff inside the !readonly block. The "Modified By" field is almost always(?!) added with the unset filter and readonly attribute, so no modal code is really needed here and neither is the hidden input field.
Long story short: Any objections?
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Administration Templates (admin) Layout Libraries |
What do you mean with "Why not"?
I agree with @matricular that the placeholder is wrong. "Select a user" cannot be performed if there is nothing to select.
When creating a new item, it would be nice if the modification fields are not shown at all.
Despite of this, I've tested the patch successfully (on 3.6.4) in the banner component - it works as described in isis and hathor and removes the placeholder.
In the Banner component the selection of an author does not work while creating a new item - neither before the patch nor after the patch, so this is another issue.
Concerning the Mootools and jQuery - someone with more experience is needed.
Sorry maybe I am being an idiot but I do not understand what you are talking about when you say you can not select a user - see the movie below
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
Sorry I get it now you just meant the "modified by Field not any of the others
Finally I understand what you were referring to but I cant help but feeling that this is the wrong way to fix it. It feels like using a sledgehammer to crack a nut
I have tested this item
I think the "summary of changes" in this PR is wrong (or too modest).
Sure it "fixes" the wrong "Select a User" string, but a better description would be, that this patch fixes a lot of small code quality issues and make the code a lot more readable.
In this sense I fully agree with this PR (and I'm looking forward to see this improvements in the 100 other layouts, too)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
To be perfectly honest. This is nothing to have "feelings" for.
The ternary overuse is a bad habbit and, apart from being hard to read and / or maintain, raises the NPath and cyclomatic complexity of a the code. The philosophy behind that is to get rid of else blocks and initialise variables with a prefered default value.
Fixing a return value for a method that, being in the cascade of files responsible for displaying the field, to match the description in its doc block, is the thing to do when you notice it. The same goes for renaming (correcting) the variable name in the doc block and all the other corrections / improvements I made in this PR.
As an example: this would show up for someone working with a custom user layout, if I had not removed the obsolete variables.
Don't get me wrong, I will happily explain my intensions for the changes to anyone, after spending hours putting everything together though, to me, it seems rude to see a question like "Why not", just minutes after opening the PR.
And now opening another PR that dublicates code blocks all together, ...
The Why Not referred to the quoted text (which possibly didnt get dispayed in your email client) and as you see later it referred to a misunderstanding as I thought you were talking about a different field
Can you explain why you made this change
Change htmlspecialchars($VARNAME, ENT_COMPAT, 'UTF-8') to $this->escape($VARNAME)
If I remember correctly there were very specific reasons for this
Yes, it was to make sure that UTF8 is used for htmlspecialchars in PHP versions below 5.4.
As well as the JViewLegacy class, JLayoutBase has a method called escape which does exactly that:
https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/layout/base.php#L114
It was used in the user field layout here:
https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/form/field/user.php#L82
So instead of having to change every occurrence of htmlspecialchars($VARNAME, ENT_COMPAT, 'UTF-8') you could use the escape method and only have to change one place (JLayoutBase) if the implementation should change for whatever reason.
Trivia:
There are debates in the interwebs if whether or not one should create proxy methods for core PHP functions but, for the unforeseeable future, that method is available in JLayoutBase, JViewLegacy and some more to be used for exactly that purpose.
Yes, it was to make sure that UTF8 is used for htmlspecialchars in PHP versions below 5.4.
The supported minimum version of Joomla is 5.3.10
Yes, I said that already.
So you cannot remove it
Remove what? Did you not understand any of what I just explained? (no offense)
If you have any questions regarding programming principles like DRY, feel free to contact me in Glip and I will try my best to explain them, but please - let's not make this discussion here any more daunting.
The place holder is wrong. There is no doubt about it.
I agree with @chmst When creating a new item, it would be nice if the modification fields are not shown at all. But as we use this form for modification too, this is a good compromise. Perhaps it would be nice to fill it with a text like “will be filled automatic”. But the solution here is an improvement because it corrects something that is wrong at the moment.
I've tested the patch successfully in the Banner and the News Feeds component - it works as described. I tested it in isis and hathor. It removes the place holder but the text in “created by” remains.
Perhaps I understood something wrong, but for me in the Banner component the selection of an author works with and without the patch while creating a new item.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
It removes the place holder but the text in “created by” remains.
this is correct as this is a filed where you can change the value from the backend ;)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
| Milestone |
Added: |
||
| Status | Pending | ⇒ | Ready to Commit |
| Milestone |
Added: |
||
RTC for 3.7.0 thanks.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
| Labels |
Removed:
?
|
||
Sorry but I still disagree with this PR
Why?
RE: this->escape vs htmlspecialchars see: https://github.com/joomla/joomla-cms/blob/3.6.4/libraries/joomla/view/html.php#L78
it uses the same code so no problem
I've retested this item successfully. Why do you disagree?
Looks like working fine here.
| Status | Ready to Commit | ⇒ | Needs Review |
| Labels | |||
I'm putting this into needs review so I (or another maintainer) can do a code review before it gets merged
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.
This looks good to me :) I've commented one thing that needs changing (that's pretty small) but I'm happy for this to be merged when it's done
| Status | Needs Review | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-12-08 23:44:08 |
| Closed_By | ⇒ | wilsonge |
Thanks @matrikular :)
cpfeifer
-
Why not?