Problem/Motivation

#2845485: Refactor and document the MenuLinkParent process plugin recently landed (yay). However, @benjifisher points out that we added this code sample:

 * Example:
 *
 * @code
 * process:
 *   parent:
 *     plugin: menu_link_parent
 *     source:
 *       - 20
 *       - management
 *       - admin/structure
 * @endcode

We should have source keys for those 3 values, not the raw values themselves.

Steps to reproduce

  1. Open core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php with a text editor.
  2. Read the opening PHPDoc comment for the class.

Proposed resolution

Use something valid in this example code block. E.g. working usage from core/modules/menu_link_content/migrations/d7_menu_links.yml: 

  parent:
    plugin: menu_link_parent
    source:
      - plid
      - '@menu_name'
      - parent_link_path

Remaining tasks

  1. Do it.
  2. Reviews/refinements.
  3. RTBC.
  4. Commit.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#11 3164120-11.patch1.18 KBdww
#7 3164120.5_7.interdiff.txt1.27 KBdww
#7 3164120-7.patch1.56 KBdww
#5 3164120-5.patch1.58 KBdww
#11 3164120.7_11.interdiff.txt1.49 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Title: Fix sample code in » Fix sample code in MenuLinkParent.php class documentation

Whoops. ;)

dww credited benjifisher.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Crediting @benjifisher for finding this and pointing it out via Slack

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3158562: Consistently refer to "menu links" as that, not "menu items", in the UI
FileSize
3164120-5.patch1.58 KB

Also, with an eye to #3158562: Consistently refer to "menu links" as that, not "menu items", in the UI, we should consistently refer to these as "menu links" not "menu items" or "menu item links", etc. ;)

Here's a patch to get us started. If we're really going to use '@menu_name' it seems like the example should show 'menu_name' being defined earlier in the migration. Perhaps that's bloat. I'm flexible. Feel free to shred this to pieces. ;)

Thanks,
-Derek

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work

Good find by benjifisher!

The patch looks good. Just a few things to make it consistent in the language.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -37,17 +37,27 @@
+ *   menu_name:

    I'm not convinced that this is needed. But it also makes the example more complete, so that is good. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -37,17 +37,27 @@
+ * 'plid' in the source (e.g. a value like '20'). If a parent menu link can't be

    I think this can be simpler, (e.g. '20') and not lose meaning. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
@@ -37,17 +37,27 @@
+ * process phase, something like 'management') and a parent menu link path (the

    s/something like/e.g./

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs work » Needs review
FileSize
3164120-7.patch1.56 KB
3164120.5_7.interdiff.txt1.27 KB

Thanks for the quick review, @quietone! Re #6:
1. Leaving alone for now.
2. Fixed.
3. Fixed.

Cheers,
-Derek

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@dww, thanks for making those changes. I am happy with this now.

I would set to RTBC but I'd like another set of eyes on it because we dww and I missed this in the earlier patch.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer commented

Thanks for the quick action on this issue, @dww and @quietone.

  1. + *   menu_name:
+ *     -
+ *       plugin: migration_lookup
+ *       migration: d7_menu
+ *       source: menu_name
+ *     -
+ *       plugin: skip_on_empty
+ *       method: row
  *   parent:
  *     plugin: menu_link_parent
  *     source:
- *       - 20
- *       - management
- *       - admin/structure
+ *       - plid
+ *       - '@menu_name'
+ *       - parent_link_path

    I think that adding the menu_name target makes the example more realistic but also more complicated. Let's just replace '@menu_name' with menu_name and rely on the comment that follows. 

  2. + * In this example we first try to look up a menu link that had an ID defined by
+ * 'plid' in the source (e.g. '20'). If a parent menu link can't be found with
+ * this ID, we try to determine the parent by a combination of a menu name (in
+ * this case, the 'menu_name' value determined earlier in the same process
+ * phase, e.g. 'management') and a parent menu link path (the 'parent_link_path'
+ * from the source, e.g. 'admin/structure').

    I was taught that "e.g." is always followed by a comma. Ideally, it should also be italicized, but that is not an option here.

  3. Suggestion: shorten "we first try to look up" to ", first look for" (note the comma).
  4. Is this the way it works: plid represents an ID in the source? So the process plugin does an implicit migration_lookup? That is weird, but fixing it would be out of scope and disruptive.
  5. Suggestion: shorten "If a parent menu link can't be found with this ID, we try" to "If that fails, try"
  6. After (1), we can simplify the parenthetical comments to just "(e.g., '...')".

Putting all those suggestions together, this is my suggested comment (after the @endcode):

In this example, first look for a menu link that had an ID defined by 'plid' in the source (e.g., '20'). If that fails, try to determine the parent by a combination of a menu name (e.g., 'management') and a parent menu link path (e.g., 'admin/structure').

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer commented
Status: Needs review » Needs work
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs work » Needs review
FileSize
3164120-11.patch1.18 KB
3164120.7_11.interdiff.txt1.49 KB

Thanks, @benjifisher: that all sounds great to me. Definitely more succinct and clear.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Issue tags: +Bug Smash Initiative

Reviews/refinements. ;)

RTBC?

Thanks,
-Derek

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@dww, thanks for the update.

Yes, that looks good to me: RTBC.

catch’s picture

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

Committed f5f925c and pushed to 9.1.x. Thanks!

  • catch committed f5f925c on 9.1.x 
    Issue #3164120 by dww, benjifisher, quietone: Fix sample code in...

Status: Fixed » Closed (fixed)

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