Support from Acquia helps fund testing for Drupal Acquia logo

Comments

metzlerd’s picture

Assigned: Unassigned » metzlerd

Picking this up next.

metzlerd’s picture

Status: Active » Needs review
FileSize
1.71 KB

Here's the first take. After looking at Token value, it didn't seem to need a usage example because it sounds like you never need to use it?

jhodgdon’s picture

Status: Needs review » Needs work

Sorry for the delay in reviewing -- I've been on vacation for two weeks.

So, this looks pretty good! I agree that Token doesn't need additional docs.

A couple of questions/comments:

  1. +++ b/core/lib/Drupal/Core/Render/Element/Hidden.php
    @@ -12,6 +12,16 @@
    + * - #value: The value of the form element that cannot be edited by the user.
    + * - #default_value:  The initial value of the form element. JavaScript may be
    + *   used to alter the value prior to submission.
    

    The relationship between value and default value doesn't make sense to me in this documentation.

    Actually does a hidden element have #value at all? I am not sure what it would do.

  2. +++ b/core/lib/Drupal/Core/Render/Element/Value.php
    @@ -10,9 +10,17 @@
    + * the browser in a hidden form field, but is just stored for use in validation
    + * and submit processing.
    

    Could we say:

    ... but is only stored in the form array for use in...

metzlerd’s picture

No worries... I'm out on a motorcycle adventure this week myself... and I was expecting you to be unavailable.

Let me try and explain. If you set #value, the value of the hidden form element will show up for the browser js to use, but the values returned by $form_state will be immutable. You could make the argument that with HTML5 people should just use data- attributes in other form elements, but this is the way #value has been used with hidden elements in the past. It might be useful to to know that this is in fact (or was in d7) true for all elements. You can set "#value" on a textfield and it will become immutable as well, even though the browser will still show the field and it will look like the user can edit it.

I realize this is a tricky thing to explain and welcome any insights or suggestions. I suppose that we could just not document the #value behavior, but I have had enough accidental uses of #value when I meant to use #default_value with hidden elements that I thought it worth mentioning in the docs.

Will certainly address #2 when I can.

jhodgdon’s picture

Ah. If #value behavior is for all elements, then I think we should put it somewhere else, not on the Hidden form element docs.

metzlerd’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
1.64 KB

Ok. Here's the changes as requested.

FYI: No one would probably ever use #value on a visible element as #disabled would be a much better choice.

jhodgdon’s picture

Status: Needs review » Needs work

Looks great, except:

+++ b/core/lib/Drupal/Core/Render/Element/Hidden.php
@@ -12,6 +12,15 @@
+ * $form['entity_id'] - array('#type' => 'hidden', '#value' => $entity_id);

This example still uses #value and not #default_value... not sure what to do here, but there is a disconnect.

metzlerd’s picture

hmm... This brings me back to feeling the need to document the distinction. I started searching for other examples in core by searching for all occurrences of '#type' => 'hidden'. Most of the examples use #value for things like entity_id, but some use it for Javascript behaviors. The Vertical Tabs control uses a default_tab hidden element so that you could keep track of the currently selected tab across form rebuilds. The books module uses it to keep track of a tree hash to help you understand if you have changed the tree via javascript when you finally do the form submission. (It actually uses #value and #default_value in the same forms on different elements for the original and modified hashes)

If you choose the wrong implementation here the code will simply break, and it seems to me that choosing #value vs. #default value is important to this control in a way that it is not to other controls. I cannot think of another control where you would normally make a choice as to whether to use #value vs. #default value.

What are your thoughts?

jhodgdon’s picture

Yeah. OK. I am convinced - we should document #value for hidden.

metzlerd’s picture

Status: Needs work » Needs review
FileSize
787 bytes
1.81 KB

Here's a rewording that might be clearer.

jhodgdon’s picture

Status: Needs review » Needs work

Looks good, just a couple of typos and I think we're done:

  1. +++ b/core/lib/Drupal/Core/Render/Element/Hidden.php
    @@ -12,6 +12,19 @@
    + * - #value: The value of the form element. The form api ensures that this value
    

    api -> API

  2. +++ b/core/lib/Drupal/Core/Render/Element/Hidden.php
    @@ -12,6 +12,19 @@
    + * $form['entity_id'] - array('#type' => 'hidden', '#value' => $entity_id);
    

    I think the - in this line should be an = right?

  3. There's a line like this in Value as well.
metzlerd’s picture

metzlerd’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Sorry, apparently my item 3 wasn't clear:

+++ b/core/lib/Drupal/Core/Render/Element/Value.php
@@ -10,9 +10,17 @@
+ * $form['entity_id'] - array('#type' => 'value', '#value' => $entity_id);

This line still has - in place of =

Sorry I didn't get that across in my last review.

Also if we're fixing that, you could change "form API" to "Form API", which I didn't say in my last review either.

metzlerd’s picture

You were clear enough. I just missed it in my haste, sorry. Here ya go.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

jhodgdon’s picture

Issue tags: +rc eligible

Tagging for "it's OK to commit now". Just API docs.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed af97e79 and pushed to 8.0.x. Thanks!

  • alexpott committed af97e79 on 8.0.x
    Issue #2486993 by metzlerd, jhodgdon: Document Hidden Form Elements
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.