Problem/Motivation

MenuLinkDefaultForm::extractFormValues() is documented as returning the plugin definition, but the ID (part of the definition) is not included.

Proposed resolution

Make sure the returned definition includes a value for ID and every other definition key.

Remaining tasks

  • \Drupal\Core\Menu\Form\MenuLinkFormInterface::extractFormValues should document that it has to return the plugin ID as well
  • Write some unit test for \Drupal\Core\Menu\Form\MenuLinkDefaultForm::extractFormValues to ensure the plugin ID is returned

User interface changes

n/a

API changes

n/a

Data model changes

Additional array keys present in return value

CommentFileSizeAuthor
#12 interdiff.txt2.2 KBdawehner
#12 2660486-12.patch4.47 KBdawehner
#11 increment.txt572 bytesrrrob
#11 2660486-11.patch2.28 KBrrrob
#5 increment.txt974 bytespwolanin
#5 2660486-5.patch1.56 KBpwolanin
#4 increment.txt817 bytespwolanin
#4 2660486-3.patch1.35 KBpwolanin
#2 2660486-2.patch1.29 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2660486-2.patch1.29 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2418031: Remove MenuLinkContentForm::doValidate() since the logic was moved to LinkWidget::validateUriElement()
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
FileSize
2660486-3.patch1.35 KB
increment.txt817 bytes

Small fix to make sure the ID is consistent.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
FileSize
2660486-5.patch1.56 KB
increment.txt974 bytes

Plus some code comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I'm wondering whether we can have some form of test coverage to ensure this is actually working as expected?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Needs work

Note: We should also make it clear in the documentation that the method is supposed to return the plugin ID as well.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Linked the related issue.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Needs tests

We also need tests.

rrrob’s picture

rrrob CreditAttribution: rrrob at Chapter Three commented
FileSize
2660486-11.patch2.28 KB
increment.txt572 bytes

Add \Drupal\Core\Menu\Form\MenuLinkFormInterface::extractFormValues documentation.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2660486-12.patch4.47 KB
interdiff.txt2.2 KB

Added a test as well

jibran’s picture

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

Thanks for the tests.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed af1f11e on 8.1.x 
    Issue #2660486 by pwolanin, dawehner, rrrob: MenuLinkDefaultForm::...

  • catch committed cad24a7 on 8.0.x 
    Issue #2660486 by pwolanin, dawehner, rrrob: MenuLinkDefaultForm::...
rrrob’s picture

rrrob CreditAttribution: rrrob at Chapter Three commented

@pwolanin in my tests, $form_state->getValue('id') is sometimes null, overwriting $new_definition['id'] which actually has a valid id.

In what cases were you seeing $new_definition['id'] not being present?

Status: Fixed » Closed (fixed)

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