Problem/Motivation

Looking at the configuration schema, I can see a few things we could improve...

  1. Continuing to support D9.0, D10.0, and D11.0, we may be able to change: - done
    1. sitemap.settings.path from type string to type path - done
    2. sitemap.settings.plugins.*.settings.rss from type string to type path - done
  2. If we dropped support for D9 and D10.0 through 10.2, (i.e.: support only D10.3+ and D11.0+ only, we could change sitemap.settings.plugins.*.weight from type integer to weight — split into #3526627: Change sitemap.settings.plugins.*.weight from type integer to weight
  3. If we dropped support for D9 and D10.0 through 10.1 (i.e.: support 10.2+), we could simplify the configuration form by making use of #config_target in our main config form. — split into #3526634: Start using #config_target in SitemapSettingsForm

Proposed resolution

Gather feedback on #3465600: Drop support for Drupal 9.x, 10.0 and 10.1 to see which new config features that we can use.

Use whichever new config features we can.

Remaining tasks

  1. Write a patch for the string → path changes
  2. Review and feedback for the string → path changes
  3. RTBC and feedback for the string → path changes
  4. Commit the string → path changes
  5. Gather feedback on #3465600: Drop support for Drupal 9.x, 10.0 and 10.1 to see which features we can use (not a blocker for this issue though!)
  6. Write a patch for any remaining changes — not going to do, we've split the remaining issues into #3526627: Change sitemap.settings.plugins.*.weight from type integer to weight and #3526634: Start using #config_target in SitemapSettingsForm

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork sitemap-3465601

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

mparker17 created an issue. See original summary.

mparker17 opened merge request !29

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Active » Needs review

Okay, I’ve decided to create a merge request for the “safest” change, i.e.: the one that only makes changes that maintain compatibility with Drupal 9.0, i.e.: changing certain types from string to path.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs review » Active

Merging the string → path changes.

Moving back to "Active" for the rest of the proposed changes

  • mparker17 committed cbc69f18 on 8.x-2.x 
    Issue #3465601 part 1: Improve configuration schema and how we use it
mparker17’s picture

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

Note that part 1 of this change was released in sitemap-8.x-2.0-beta7; but there are still outstanding parts of this issue to be released in the future.

dcam’s picture

dcam commented

From my own comment in #3465577-18: Split sitemap_book config schema from sitemap schema:

I'm guessing that a lot of the nullable keys were the result of all plugin settings being lumped into a single array. You had to do that so one plugin could leave all the other settings as empty.

While this config improvement is going on, any unnecessary nullable properties should be removed.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Active » Needs work

Updating the issue summary: we are unblocked now, so moving to Needs Work.

mparker17’s picture

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

I split off...

2. If we dropped support for D9 and D10.0 through 10.2, (i.e.: support only D10.3+ and D11.0+ only, we could change sitemap.settings.plugins.*.weight from type integer to weight

... into its own issue, #3526627: Change sitemap.settings.plugins.*.weight from type integer to weight; updating issue summary to say this.

mparker17 changed the visibility of the branch 3465601-config-schema-2 to hidden.

mparker17’s picture

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

I split off...

3. If we dropped support for D9 and D10.0 through 10.1 (i.e.: support 10.2+), we could simplify the configuration form by making use of #config_target in our main config form.

... into its own issue, #3526634: Start using #config_target in SitemapSettingsForm; updating the issue summary to say this.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs work » Fixed

I think that, since we've split the remaining tasks into #3526627: Change sitemap.settings.plugins.*.weight from type integer to weight and #3526634: Start using #config_target in SitemapSettingsForm, we can mark this as fixed.

Status: Fixed » Closed (fixed)

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