Problem/Motivation

In #3032084: Add an option for menu depth we introduced a menu_depth option for sitemap menu plugins, but forgot to set a default for the option.

In Drupal CMS's PHPUnit test, the latest release of sitemap has broken us due to an undefined array key (which will fail PHPUnit tests):

Exception: Warning: Undefined array key "menu_depth"
Drupal\sitemap\Plugin\Sitemap\Menu->view()() (Line: 116)

Proposed resolution

This seems like a minor oversight; probably what's needed is to ensure that menu_depth has a default value in the class's #[Sitemap] attribute.

Remaining tasks

  1. Write a patch - done by @phenaproxima in #2
  2. Review and feedback - done by @mparker17 in #5
  3. RTBC and feedback - done by @mparker17 in #5
  4. Commit - done by @mparker17 in #7
  5. Release - released in 8.x-2.2

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork sitemap-3541905

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima opened merge request !48

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

For the purposes of documentation, the menu_depth option was introduced in #3032084: Add an option for menu depth, which included sitemap_update_8205() to update existing configuration.

Instructions for running sitemap_update_8205() were mentioned in the Upgrade instructions section of the 8.x-2.1 release notes (I took time writing that section, because there's been some confusion after previous releases about how to upgrade the module — see #3466939: Route "sitemap.page" does not exist error after upgrading to 8.x-2.0-beta7)

As a module maintainer, I'm curious to know what I could do to avoid this in the future. That is to say, what is Drupal CMS testing that I'm not testing? How could I improve test coverage to avoid this?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs review » Reviewed & tested by the community

Reviewing the code, it looks good to me.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Assigning attribution.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@phenaproxima, it has been merged!

Can I assume you'd like me to make a bugfix release right away?

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Updating status and issue summary so it's easy to figure out what happened in the future.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

@mparker17, thanks for merging this! An immediate bugfix release is not necessary unless you really want to; this is currently only affecting Drupal CMS's tests -- I doubt it poses any problem for live sites. Therefore we have no problem patching it for internal development purposes.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

I released sitemap-8.x-2.2 today with this fix. Thanks @phenaproxima!

Please do let me know if there's anything that I can do to improve my test coverage so I don't break Drupal CMS tests in the future!

Status: Fixed » Closed (fixed)

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