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

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2660486-2.patch1.29 KB
wim leers’s picture

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

pwolanin commented
StatusFileSize
new 2660486-3.patch1.35 KB
new increment.txt817 bytes

Small fix to make sure the ID is consistent.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 2660486-5.patch1.56 KB
new increment.txt974 bytes

Plus some code comments

dawehner’s picture

dawehner
Primary language German
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
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
commented

Linked the related issue.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Needs tests

We also need tests.

rrrob’s picture

rrrob commented
StatusFileSize
new 2660486-11.patch2.28 KB
new increment.txt572 bytes

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

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2660486-12.patch4.47 KB
new interdiff.txt2.2 KB

Added a test as well

jibran’s picture

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

Thanks for the tests.

catch’s picture

catch
he/him
Primary language English
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 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.