- last update
almost 2 years ago Patch Failed to Apply - last update
almost 2 years ago Patch Failed to Apply - First commit to issue fork.
- last update
over 1 year ago 22 pass - π¬π§United Kingdom welly
Created a new patch/fork because the patch above doesn't seem to apply on the latest version.
- Status changed to RTBC
about 1 year ago 10:07pm 30 March 2024 - Status changed to Needs work
about 1 year ago 2:04am 31 March 2024 - π¨π¦Canada mparker17 UTC-4
This looks great, but I don't want to merge new functionality without tests.
- First commit to issue fork.
- π¬π§United Kingdom nexusnovaz
nexusnovaz β changed the visibility of the branch 3032084-add-an-option to hidden.
- π¬π§United Kingdom nexusnovaz
I was trying to write some tests unfortunately I can't seem to get PhpStorm to work with it. I've rolled #12 into the MR !39. I'm going to keep trying to see if i can get mine to work, but if anyone gets it first, feel free.
- π―π΅Japan tom konda Kanagawa, Japan
tom konda β made their first commit to this issueβs fork.
- π―π΅Japan tom konda Kanagawa, Japan
I rebased feature branch and added a test for menu depth option.
But, need to fixSitemapUpdateSettingsTest::testUpdateHookN
. - π¨π¦Canada mparker17 UTC-4
Putting on my maintainer hat, thank you, @nexusnovaz and @tom konda!
SitemapMenuTest::testMenuDepth()
looks good to me: thank you very much!Putting on my developer hat, I think that
SitemapUpdateSettingsTest::testUpdateHookN()
is failing because the newmenu_depth
configuration option needs to be added to the configuration schema inconfig/schema/sitemap.schema.yml
β at time-of-writing, there is a schema for Menu plugins between lines 67 and 74 of that file.I wanted to provide an example of what that configuration might look like, but I could not find an example, because this merge request uses the string
unlimited
to indicate unlimited menu depth...- I searched Drupal core and some contrib modules (
Admin Toolbar β
,
Better Exposed Filters β
,
Client-side Hierarchical Select β
) for the search term
depth
in all*.schema.yml
files, to see there were any other examples of "depth" configuration that could either be a string or an integer, but I could not find any examples. - It seems like most "menu depth" configuration uses
type: integer
, where the integer9
indicates the maximum possible depth. - This module's
sitemap_plugin_settings.vocabulary
schema defines bothterm_depth
andrss_depth
astype: integer
, but useNULL
to indicate unlimited depth (i.e.: by saying bothtype: integer
andnullable: true
in the config schema). - Other examples of "string or integer" configuration that I could think of (e.g.:
system.performance:cache.page.max_age
,automated_cron.settings:interval
,dblog.settings:row_limit
,system.file:temporary_maximum_age
, etc.) use a special integer (0
,-1
) to indicate the string "Unlimited", "Never", "All", etc. That being said, I might have missed something!
In summary, it might be simpler to write a config schema if you changed the merge request to use an integer like
-1
, or the valueNULL
to indicate an unlimited depth, instead of what the merge request does now (i.e.: use the stringunlimited
to indicate an unlimited depth).For what it's worth, I've found the Configuration Inspector module β to be helpful when writing and debugging configuration schemas.
- I searched Drupal core and some contrib modules (
Admin Toolbar β
,
Better Exposed Filters β
,
Client-side Hierarchical Select β
) for the search term