Problem

Postponed on #2407505: [meta] Finalize the menu links (and other user-entered paths) system

Shortcut entity validation misses the logic from ShortCutForm::validate(). This is critical as the validation logic will be missed from RESTful validation. (see #1696648: [META] Untie content entity validation from form validation)

Proposed resolution

Convert the logic to entity validation and leverage entity validation in form validation.

Remaining tasks

Implement proposed resolution.

User interface changes

-

API changes

-

CommentFileSizeAuthor
#24 d8_shortcut_fail.patch2.1 KBfago
#20 shortcut-entity-validation-2403847.20.patch7.51 KBlarowlan
#20 interdiff.txt2.57 KBlarowlan
#18 shortcut-entity-validation-2403847.18.patch6.79 KBlarowlan
#18 interdiff.txt3.77 KBlarowlan
#4 2403847-4.patch5.04 KBdawehner
#2 2403847-2.patch49.79 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Component: menu system » shortcut.module
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
2403847-2.patch49.79 KB

Tried on of the conversions in order to be able to review other ones.

In general though this can't be right, given that its incredible hard to pull the value you want to validate. $value
passed into the validator though for itself is NULL, at least in the form.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Looks like the patch contains some the bulk action access changes as well, hard to find the relevant parts :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
2403847-4.patch5.04 KB

These little details ;)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

UserNameConstraintValidator might be a useful example to look at, according to that, the value you get are the $items.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented 
  1. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraintValidator.php
@@ -0,0 +1,50 @@
+    $typed_data = $this->context->getMetadata()->getTypedData();
+    $name = $typed_data->getDataDefinition()->getMainPropertyName();
+    $value = $typed_data->first()->get($name)->getValue();

    If what you want to validate is the 'value' property only, you could just add your constraint via setPropertyConstraints() to value - then you'll just get that value passed. 

  2. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraintValidator.php
@@ -0,0 +1,50 @@
+    if (!$this->pathValidator()->isValid($value)) {

    Ususally, validatiors have a check for $value bing NULL and return doing nothing if so, such that NULL values go through fine / no double violations happen when a required field is missing.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

UserNameConstraintValidator might be a useful example to look at, according to that, the value you get are the $items.

Yes, except the field is empty - then you'll get NULL.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I tried to use ->setPropertyConstraints('value', ['ShortcutPath'])but this does not trigger the validator at all,
maybe the arguments are a little bit different?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: +Needs reroll, +Critical Office Hours

Good one for re-roll in today's office hours

Pinolo’s picture

Pinolo
he/him
CreditAttribution: Pinolo commented
Assigned: Unassigned » Pinolo
Issue tags: +SprintWeekend2013
Pinolo’s picture

Pinolo
he/him
CreditAttribution: Pinolo commented

@fago: isn't setPropertyConstraints() supposed to go away, as per https://www.drupal.org/node/2335633 ?

alimac’s picture

alimac CreditAttribution: alimac commented
Issue tags: -SprintWeekend2013 +SprintWeekend2015

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

manningpete’s picture

manningpete CreditAttribution: manningpete commented
Issue tags: -Needs reroll

I could apply the patch, so no reroll is needed.

Pinolo’s picture

Pinolo
he/him
CreditAttribution: Pinolo commented
Status: Needs review » Needs work

I think the issue about $value being always NULL should be investigated more thoroughly, which is what I'm trying to do now. Moreover, I find very strange that, if I leave blank the path value, when I reach the ShortcutPathConstraintValidator->validate() function, the value has already been altered to "<front>".

Pinolo’s picture

Pinolo
he/him
CreditAttribution: Pinolo commented

After going through the issue queue, I think this issue should depend on #2235457: Use link field for shortcut entity (thus making that one critical, too). Turning the path field into a "standard" link field should make some parts of this patch obsolete. As @berdir puts it in the other issue's description: "Shortcut still has (pretty hacky) separate fields for that".

Quick recap:
* given the hackiness of the shortcut path field implementation, there can only be a hacky way of getting the value to validate
* as of now, the extracted value can never be NULL or empty because, before arriving here, ShortcutPathValue::setValue() kicks in

Pinolo’s picture

Pinolo
he/him
CreditAttribution: Pinolo commented
Assigned: Pinolo » Unassigned
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
  1. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraint.php
@@ -0,0 +1,30 @@
+ * Contains Drupal\shortcut\Plugin\Validation\Constraint${NAME}.

    Wrong class name. Also first back slash missing. 

  2. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraintValidator.php
@@ -0,0 +1,50 @@
+ * Contains Drupal\shortcut\Plugin\Validation\Constraint\ShortcutPathConstraintValidator.

    First black slash missing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
Issue tags: +CriticalADay
FileSize
interdiff.txt3.77 KB
shortcut-entity-validation-2403847.18.patch6.79 KB

so yeah, path is a computed field so the $items are always null, so we need to go the typed-data/metadata way

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Thanks for the fixes. Here are some minor points. It is ready imo.

  1. +++ b/core/modules/shortcut/src/Tests/ShortcutValidationTest.php
@@ -0,0 +1,60 @@
+    $this->installEntitySchema('shortcut');
+    $this->installEntitySchema('shortcut_set');
+    $this->installConfig(['shortcut']);

    Isn't it all installed as a part of $modules? 

  2. +++ b/core/modules/shortcut/src/Tests/ShortcutValidationTest.php
@@ -0,0 +1,60 @@
+  public function testValidation() {

    We can try admin patch. We can try entity path.
    We can try extrnal url.
    In other words we can have more asserts :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
interdiff.txt2.57 KB
shortcut-entity-validation-2403847.20.patch7.51 KB

no worries

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

Awesome!

  • alexpott committed 4a3abdc on 8.0.x 
    Issue #2403847 by larowlan, dawehner: Shortcut content entity validation...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 4a3abdc and pushed to 8.0.x. Thanks!

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Fixed » Needs work
FileSize
d8_shortcut_fail.patch2.1 KB

Looking at the committed patch, I'm sry I think I've to re-open this. Problem is, the validated path field is computed - but computed fields are skipped on entity validation. This is green and works as it always directly validates the path field, but any logic invoking $entity->validate() would miss it.

Also, I found some other not so critical remarks:

  1. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraint.php
@@ -0,0 +1,30 @@
+ *   type = { "entity" }

    This is wrong. We can just leave it out - it's not useful for custom constaints and needs to be fixed for lists. 

  2. +++ b/core/modules/shortcut/src/Plugin/Validation/Constraint/ShortcutPathConstraintValidator.php
@@ -0,0 +1,54 @@
+    // We cannot use $items here because this is a computed field, we need to
+    // fetch the value direct from TypedData.
+    $list = $this->context->getMetadata()->getTypedData();

    From reading, I did not understood why I must say. But it has to do with computed fields being skipped, I'd assume.

    Anyway, the logical level to put this would be the field item imo, as it really just validates a single item - not a list items. That way, the constraint would apply to theoritcally multi-value field properly and there should be no need to fetch the typed data object.

    Tried to convert it and ran into the issue of validation being skipped -as this is all computed. (see above)

    Anyway, I attached the patch I started. Not sure what's the best way to fix here. Imho, not running validation on computed things makes sense - it's computed, not user inputtted data. So it's this field which weirdly is not always computed: It only computes the value, if there is none set. Not sure how to fix this best yet.

    Maybe we could fix validation of computed fields, such that we only run it as soon as custom constraint has been added?

  • alexpott committed b4f1849 on 8.0.x 
    Revert "Issue #2403847 by larowlan, dawehner: Shortcut content entity...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Okay I've reverted so we can get more discussion about #24 - not I also discussed this patch wrt to the current discussion on path and menu link storage. I felt that committing this was the best way to go because it added tests that would be helpful if shortcut was converted to use the new storage.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review

bot review of #24

Status: Needs review » Needs work

The last submitted patch, 24: d8_shortcut_fail.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Postponed

To be honest I think the answer is postpone this until #2407505: [meta] Finalize the menu links (and other user-entered paths) system and #2235457: Use link field for shortcut entity are done.
After that is might no longer be a computed field

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Yes, that fixes the issue with computed field validation. Agreed, this is the right way forward then.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Title: Shortcut content entity validation misses form validation logic » [PP1] Shortcut content entity validation misses form validation logic
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Just one additional note since this is already reverted. This might be moot if this gets reimplemented in some way, but the current ShortcutPath constraint and validator in the patch are completely generic (they only rely on Core's PathValidator) so there's no reason for them to live in shortcut module. Validating paths on entities will be pretty useful for contrib.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@tstoeckler
Good point

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Title: [PP1] Shortcut content entity validation misses form validation logic » Shortcut content entity validation misses form validation logic
Status: Postponed » Closed (duplicate)

#2235457: Use link field for shortcut entity got in and now ShortcutForm does not do any custom validation anymore. The link field uses Drupal\link\Plugin\Validation\Constraint\LinkTypeConstraint so I think this issue can be closed.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Agreed - great to see this solved that way.