Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by) #12627

matrikular · 2016-10-29T21:30:22Z

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?

…odified_by) Additional changes: - Remove obsolete extracted variables that are not present for this field type - Correct variable name in doc block to match extracted name - Refactor code to remove ternary overuse

@SInCE

Additional changes: - Satisfy Code Sniffer (e.g. @SInCE) - Remove else block after variable initialisation

…pe($VARNAME) Additional changes: - Change JHtml::script() call to JHtml::_('script' ...) to allow overrides via custom register method

…modified_by) Additional changes: - 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 - Remove obsolete extracted variables that are not present for this field type - Removed the class suffix from the hidden field - Remove (Fix) obsolete whitespace in class attribute for the visible field when no suffix was entered

brianteeman · 2016-10-29T21:59:36Z

"Select a User" placeholder. Since you cannot really select a user here,
that text is obsolete in that context.

Why not?

matrikular · 2016-10-29T22:04:08Z

What do you mean with "Why not"?

chmst · 2016-10-30T10:22:49Z

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.

brianteeman · 2016-10-30T10:36:40Z

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.}

brianteeman · 2016-10-30T10:57:34Z

Sorry I get it now you just meant the "modified by Field not any of the others

brianteeman · 2016-10-30T10:58:24Z

administrator/templates/isis/html/layouts/joomla/form/field/user.php

 *
 * @var   string   $userName        The user name
 * @var   mixed    $groups          The filtering groups (null means no filtering)
- * @var   mixed    $exclude         The users to exclude from the list of users
+ * @var   mixed    $excluded        The users to exclude from the list of users


I dont see the need to change this - it seems correct english to me

brianteeman · 2016-10-30T10:59:03Z

layouts/joomla/form/field/user.php

 *
 * @var   string   $userName        The user name
 * @var   mixed    $groups          The filtering groups (null means no filtering)
- * @var   mixed    $exclude         The users to exclude from the list of users
+ * @var   mixed    $excluded        The users to exclude from the list of users


See previous comment - the english is correct to me

It is not about correcting the word to be in the correct tense but to name the variable correctly. After "extracting" the information from $displayData, a variable $excluded is created. The doc block helps IDEs to autocomplete methods and variable names. As you can see in the original file here:
https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/form/field/user.php#L52
or the modified file here:
https://github.com/matrikular/joomla-cms/blob/49dea5de2a642a10b8735ad4185477192d87c7b8/layouts/joomla/form/field/user.php#L65
the doc block shows the wrong variable name.

OK thanks for that explanation - it makes sense

Thats the problem when you create a long PR without explaining what you have changed and why you have changed things

brianteeman · 2016-10-30T11:06:11Z

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

bembelimen · 2016-10-30T11:39:14Z

I have tested this item ✅ successfully on 49dea5d

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.}

brianteeman · 2016-10-30T11:39:29Z

Please see #12639 for an alternative approach which I think is much simpler - assuming I have addressed the correct issues

matrikular · 2016-10-30T11:41:44Z

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, ...

brianteeman · 2016-10-30T11:45:49Z

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

brianteeman · 2016-10-30T11:48:58Z

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

matrikular · 2016-10-30T12:02:27Z

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.

brianteeman · 2016-10-30T12:39:55Z

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

matrikular · 2016-10-30T12:42:04Z

Yes, I said that already.

wilsonge · 2016-10-30T12:43:13Z

libraries/cms/form/field/user.php

@@ -14,7 +14,7 @@
 *
 * @since  1.6
 */
-class JFormFieldUser extends JFormField implements JFormDomfieldinterface
+class JFormFieldUser extends JFormField


You need to add the interface back here please

@wilsonge

Missed that in the manual merge process. While I was working on the PR, the custom fields code got merged. Thanks @wilsonge.

brianteeman · 2016-10-30T12:47:51Z

So you cannot remove it

matrikular · 2016-10-30T13:03:05Z

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.

astridx · 2016-10-31T13:46:26Z

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.

…atch-15 Resolve conflicts

zero-24 · 2016-12-05T10:03:10Z

I have tested this item ✅ successfully on 617a3d9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.}

zero-24 · 2016-12-05T10:03:48Z

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.}

zero-24 · 2016-12-05T10:04:23Z

RTC for 3.7.0 thanks.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12627.}

brianteeman · 2016-12-05T10:55:39Z

Sorry but I still disagree with this PR

zero-24 · 2016-12-05T11:01:10Z

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 😄

chmst · 2016-12-05T11:04:48Z

I've retested this item successfully. Why do you disagree?

infograf768 · 2016-12-05T11:05:40Z

Looks like working fine here.

wilsonge · 2016-12-05T12:59:00Z

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.}

wilsonge · 2016-12-08T22:24:54Z

administrator/templates/isis/html/layouts/joomla/form/field/user.php

-	<input type="hidden" id="<?php echo $id; ?>_id" name="<?php echo $name; ?>" value="<?php echo (int) $value; ?>"
-		class="field-user-input <?php echo $class ? (string) $class : ''?>"
-		data-onchange="<?php echo $this->escape($onchange); ?>"/>
+	<input type="hidden" id="<?php echo $id; ?>_id" name="<?php echo $name; ?>" value="<?php echo (int) $value; ?>" class="field-user-input" data-onchange="<?php echo $this->escape($onchange); ?>" />


You need to use $inputAttributes ['class']

wilsonge · 2016-12-08T22:32:55Z

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

wilsonge · 2016-12-08T23:44:11Z

Thanks @matrikular :)

matrikular added 5 commits October 29, 2016 16:34

Changed getExcluded method return value to match doc block

4c9bea2

Additional changes: - Satisfy Code Sniffer (e.g. @SInCE) - Remove else block after variable initialisation

Change htmlspecialchars($VARNAME, ENT_COMPAT, 'UTF-8') to $this->esca…

a9c02b1

…pe($VARNAME) Additional changes: - Change JHtml::script() call to JHtml::_('script' ...) to allow overrides via custom register method

Move hidden field outside the condition for now

49dea5d

joomla-cms-bot added the PR-staging label Oct 29, 2016

brianteeman reviewed Oct 30, 2016

View reviewed changes

brianteeman mentioned this pull request Oct 30, 2016

Readonly field do not display "select user" #12639

Closed

wilsonge reviewed Oct 30, 2016

View reviewed changes

Re-Added interface

c8f9c3a

Missed that in the manual merge process. While I was working on the PR, the custom fields code got merged. Thanks @wilsonge.

Merge branch 'staging' of https://github.com/joomla/joomla-cms into p…

617a3d9

…atch-15 Resolve conflicts

zero-24 added this to the Joomla 3.7.0 milestone Dec 5, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 5, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 5, 2016

wilsonge reviewed Dec 8, 2016

View reviewed changes

Add custom class back to hidden field for potential b/c breaks

e1185ba

wilsonge merged commit 0838299 into joomla:staging Dec 8, 2016

matrikular deleted the patch-15 branch March 27, 2017 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by) #12627

Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by) #12627

matrikular commented Oct 29, 2016 •

edited by joomla-cms-bot

brianteeman commented Oct 29, 2016

matrikular commented Oct 29, 2016 •

edited

chmst commented Oct 30, 2016

brianteeman commented Oct 30, 2016

brianteeman commented Oct 30, 2016

brianteeman Oct 30, 2016

brianteeman Oct 30, 2016

matrikular Oct 30, 2016 •

edited

brianteeman Oct 30, 2016

brianteeman commented Oct 30, 2016

bembelimen commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016 •

edited

brianteeman commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016

wilsonge Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016 •

edited

astridx commented Oct 31, 2016

zero-24 commented Dec 5, 2016

zero-24 commented Dec 5, 2016

zero-24 commented Dec 5, 2016

brianteeman commented Dec 5, 2016

zero-24 commented Dec 5, 2016

chmst commented Dec 5, 2016

infograf768 commented Dec 5, 2016

wilsonge commented Dec 5, 2016

wilsonge Dec 8, 2016

wilsonge commented Dec 8, 2016

wilsonge commented Dec 8, 2016

Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by) #12627

Fix placeholder showing "Select a User" when in readonly mode (e.g. modified_by) #12627

Conversation

matrikular commented Oct 29, 2016 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Caveat

brianteeman commented Oct 29, 2016

matrikular commented Oct 29, 2016 • edited

chmst commented Oct 30, 2016

brianteeman commented Oct 30, 2016

brianteeman commented Oct 30, 2016

brianteeman Oct 30, 2016

Choose a reason for hiding this comment

brianteeman Oct 30, 2016

Choose a reason for hiding this comment

matrikular Oct 30, 2016 • edited

Choose a reason for hiding this comment

brianteeman Oct 30, 2016

Choose a reason for hiding this comment

brianteeman commented Oct 30, 2016

bembelimen commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016 • edited

brianteeman commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016

wilsonge Oct 30, 2016

Choose a reason for hiding this comment

brianteeman commented Oct 30, 2016

matrikular commented Oct 30, 2016 • edited

astridx commented Oct 31, 2016

zero-24 commented Dec 5, 2016

zero-24 commented Dec 5, 2016

zero-24 commented Dec 5, 2016

brianteeman commented Dec 5, 2016

zero-24 commented Dec 5, 2016

chmst commented Dec 5, 2016

infograf768 commented Dec 5, 2016

wilsonge commented Dec 5, 2016

wilsonge Dec 8, 2016

Choose a reason for hiding this comment

wilsonge commented Dec 8, 2016

wilsonge commented Dec 8, 2016

matrikular commented Oct 29, 2016 •

edited by joomla-cms-bot

matrikular commented Oct 29, 2016 •

edited

matrikular Oct 30, 2016 •

edited

matrikular commented Oct 30, 2016 •

edited

matrikular commented Oct 30, 2016 •

edited