Problem/Motivation

\Drupal\Core\Menu\Form\MenuLinkDefaultForm::$moduleData is never used, nor is it initialized

Proposed resolution

Remove it

Possibly implement __get to trigger a deprecation error if someone tries to access it?

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3088163-depricate-module-data-8-7.patch612 bytesamjad1233
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

amjad1233’s picture

amjad1233
MJED
Location Brisbane
CreditAttribution: amjad1233 as a volunteer and commented
Assigned: Unassigned » amjad1233
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

@larowlan asked whether we should deprecate the property and provide BC, instead of removing it.

  • In general, we prefer to use deprecation and provide BC even for internal APIs.
  • However, since this property is already not initialized, no one subclassing this form is getting useful data from us anyway -- it's just NULL. So, it should at most raise a notice.
  • If they are setting a value themselves, that will also continue to work.

Therefore, I think we should go the simple route and just remove it.

Thanks!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Active » Closed (duplicate)

Sorry for the noise, #2940189: Deprecate system_get_info() already removed it while cleaning up calls to system_get_info() - I was looking at 8.7 when I saw it.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.9.x-dev » 8.7.x-dev
Status: Closed (duplicate) » Active
amjad1233’s picture

amjad1233
MJED
Location Brisbane
CreditAttribution: amjad1233 as a volunteer and commented
Status: Active » Needs review
FileSize
3088163-depricate-module-data-8-7.patch612 bytes
amjad1233’s picture

amjad1233
MJED
Location Brisbane
CreditAttribution: amjad1233 as a volunteer and commented
Assigned: amjad1233 » Unassigned
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

This looks good to me, thank @amjad1233 - setting to RTBC on the assumption that the testbot likes it.

  • xjm committed 3cbe849 on 8.7.x 
    Issue #3088163 by amjad1233, larowlan, xjm: \Drupal\Core\Menu\Form\...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Normally, we wouldn't commit patches that have any chance of disruption to the production (patch release) branch, but in this chance it's so slight I think it's OK. Committed and pushed to 8.7.x.

Congratulations @amjad1233 on your first core issue credit!

amjad1233’s picture

amjad1233
MJED
Location Brisbane
CreditAttribution: amjad1233 as a volunteer and commented

Hi @xjm, Thanks for that. Next time I will make sure to check out the -dev branch before committing the patch.
Thanks once again.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

🎉 congrats @amjad1233

Status: Fixed » Closed (fixed)

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