[UX] Fixed: Add numeric validation on entity IDs for visibility conditions.

[olafgrabienski]

Updated issue description

I created a custom node Layout and tried to set the visibility condition of the Layout to "Node ID is node/7" (see Original issue description below). That didn't work because you have to enter just the number, e.g. 7, instead of the path.

To prevent such an error, the Node ID form of the visibility condition dialog should be validated so that only numbers can be entered, and if needed, the user should get a respective message.

Another update: Looking at the dialog again, you see "Node ID is: node/7", but when you click on "Configure", there is a 0 (zero) in the Node NID field. So, the form seems to be validated, and the value is set automatically to 0 in the case you don't enter a number, but you don't get a reasonable feedback about it.

I tested Block visibility conditions as well. They have the same issue.

---

Original issue description

When I create a custom node layout, the condition "Node ID is node/XYZ" doesn't work.

Steps to reproduce on a Backdrop 1.6.3 site:

1. Add a node, e.g. node/7.
2. Add a custom layout, Path: node/%, Visibility condition: "Node ID is node/7".
3. Add a unique block to the custom layout, just to be able to check if it's displayed.
4. Go to node/7. The custom layout isn't in use. Instead I see the content in my default node layout (path: node/%, no further conditions).

If you change the visibility condition in step 2 to "Current path is node/7", everything works fine.

As mentioned above, on the site in question, there is a more general custom node layout in use on the site:

• I changed the order of the layouts, just in case, even if the more specific should work anyway. Changing the order didn't resolve the issue.
• To check if the more general custom layout causes any problem, I disabled it. That didn't help, resulting in the display of the Default layout provided by Backdrop core.

I'd be happy if someone could try to reproduce the issue!

---

Translation update:

New Strings:

• There is no @label with ID @entity_id.

[klonos]

@olafgrabienski I was just reading the steps you were taking over on Gitter and this is what I think might be wrong: "Node ID is node/7". Node IDs are just numbers; no paths. So this condition should be just "Node ID is 7". I have not tested this, but that's what I think you're doing wrong. Need to attend the weekly dev meeting. Back with some testing after that 😄 ...

[klonos]

...but what we definitely need here is validation on the form so that only numbers are entered as NID.

[olafgrabienski]

So this condition should be just "Node ID is 7".

@klonos Thanks for your suggestion, that's absolutely true!

I'll change the issue description and title to reflect the need of form validation.

[olafgrabienski]

@klonos After some more testing I updated title and issue description again. Seems that there is a form validation, but it doesn't help really and should be improved.

[klonos]

@olafgrabienski yes, the validation should happen for both layout conditions and block conditions. I had renamed the issue and included only blocks, you renamed it to change blocks to layouts, then must have realized that these conditions apply to both and re-renamed it 👍

This is not only about NID only though. Depending on context it could be a UID or a TID. So, I am re-re-renaming to account for all entity types 😄

Also, there is no actual validation on the dialog that adds/edits the visibility conditions and this leads to the form allowing strings to be saved where only numeric values should be entered:

Notice how I was not prevented to enter "abc" as NID and when I try to edit it, the NID has fallen back to 0.

This is a bug; not a task.

[olafgrabienski]

Thanks, @klonos - I agree with all points.

Notice how I was not prevented to enter "abc" as NID and when I try to edit it, the NID has fallen back to 0.

At the screenshot notice also the difference between "Node ID is: ..." and "Node NID is ...". "Node NID" sounds quite confusing, we should use just "Node ID".

[klonos]

Yes, I think that the "Node NID" was unintentional (perhaps carried away because the drop-down menu has a "Node: NID" entry, which in the context of the drop-down does make sense).

[jenlampton]

Closing backdrop/backdrop#3759 in favor of this issue, since it was older. Thanks @bugfolder for the quick PR!

[jenlampton]

I've provided an alternate PR that changes only 2 lines, But the error messaging is a little different:

[indigoxela]

I followed the steps again, and here's what I get for a non-existent nid:

Nice! Definitely works for me.

[indigoxela]

@bugfolder A tiny coding standard improvement and then we're done.

[bugfolder]

I made that change, both in the function noted, and in three more places in the same file. (I also see the same thing in a few other access plugins, but I'll leave those alone for now.)

[docwilmot]

I'm finding that I cannot type non-numeric characters in the number field. Is this browser dependent?

[bugfolder]

Yes. Some browsers won't let you type non-numeric values, others let you type it in but then submit an empty value. Here's some discussion of that issue. #5281 (comment)

[indigoxela]

I made that change, both in the function noted, and in three more places in the same file.

Great, many thanks. This really improves code readability. 👍 RTBC!

[klonos]

@indigoxela please excuse my ignorance, but can you please point me to the coding standard about breaking long lines of functions that include arrays as parameters? I'm asking because unless I'm mistaken, I think it is best-practice and preference rather than a coding standard. (I do not disagree - just curious of whether we have this documented in our coding standards or not)

PS: another thing that I've seen done is to break the array parameter into a separate variable. Something like this:

    $replacements = array(
      '@entity' => $this->entity_info['label'],
      '@entity_id_key' => strtoupper($this->entity_info['entity keys']['id']),
    );
    $form['entity_id'] = array(
      '#title' => t('@entity @entity_id_key is', $replacements),
      ...
    );

[klonos]

@bugfolder just a couple of extraneous spaces in the PR (I've left code suggestions).

[indigoxela]

@klonos the officially published coding standards for Backdrop are very sparse. I mostly rely on phpcs, as I'm using it with both, Backdrop and Drupal.

PS: I'd never try to argue coding standard topics with you. 😜 I've learned.
Just do what you feel is appropriate.

[bugfolder]

just a couple of extraneous spaces in the PR (I've left code suggestions).

Thanks, suggestions merged into PR.

[klonos]

PS: I'd never try to argue coding standard topics with you. 😜 I've learned.

@indigoxela 😝 ...these specific were double spaces between array key/values. For example:

- '@entity'  => $this->entity_info['label'],
+ '@entity' => $this->entity_info['label'],

In general though, my process is to look in the Backdrop coding standards ...if they don't say anything, then fall back to D7 coding standards (and try to copy the D7 coding standards to our docs.b.org) ...if nothing there either, then I usually start a discussion/issue about establishing a coding standard.

[quicksketch]

I merged the follow-up PR at backdrop/backdrop#3771 into 1.x and 1.20.x. Thanks for following through on this @bugfolder! Thanks @indigoxela, @klonos, and @docwilmot for the follow-up reviews!

[klonos]

I had some half-done solution in my local instead of the odd usage of round() in the validation. I was using a combination of '#step' => 1, + adding pattern="\d*" (which is supported by all major browsers for some time now) via '#attributes' in the form element (to avoid decimals). Perhaps I'll file a follow-up PR.