Problem/Motivation

Follow up to #[3620504]. Once active-link.js stops reading drupalSettings.path.currentQuery, nothing in core sets or reads it for a reason. SystemHooks::jsSettingsAlter() still embeds $request->query->all() straight into drupalSettings on every page with a query string, with no cache context attached.

Found investigating #3011426: Page cache size is unlimited when arbitrary query parameters are requested. Keeping this around is a standing liability for anyone trying to reason about whether a page's output actually depends on the query string (e.g. contrib #3610948: Automatically cache pages that don't vary by query args, or a core equivalent) since it's an untracked dependency.

Steps to reproduce

grep core for "currentQuery". Only write site: SystemHooks::jsSettingsAlter. Only read site was active-link.js, removed in #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead.

Proposed resolution

Stop setting path.currentQuery by default in SystemHooks::jsSettingsAlter. Document the change.

Remaining tasks

  1. First merge #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead so that currentQuery is no longer used
  2. Then this issue (the 'draft' MR is a combination of this and 3620504)

Make the change + change record to note in currentQuery removal that contrib/custom JS relying on it needs to switch to window.location.search.

User interface changes

None.

Introduced terminology

None.

API changes

drupalSettings.path.currentQuery deprecated.

Data model changes

None.

Release notes snippet

drupalSettings.path.currentQuery is deprecated. Read the current query string from window.location instead.

Issue fork drupal-3620505

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

scott_euser created an issue. See original summary.

scott_euser’s picture

scott_euser commented
Status: Active » Postponed
Related issues: +#3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead

scott_euser opened merge request !16982

scott_euser opened merge request !16983

scott_euser’s picture

scott_euser commented
Issue summary: View changes
scott_euser’s picture

scott_euser commented
Issue summary: View changes
Status: Postponed » Needs review
  1. Merge request !16982 expected to fail because #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead needs merge first
  2. Merge request !16983 passes as expected because it contains #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead
mstrelan’s picture

mstrelan commented
Status: Needs review » Postponed

Let's postpone on #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead then. Wasn't sure which MR to comment on, but the @see link will need to point to the CR instead of this issue.