Handle invalid compressed ajax_page_state more gracefully

We have a similar issue, and we are NOT using Solr facets, it's entity browsers with views where ajax paging is failing.

We got to debug it to the point we found that it's trying to decompress an already decompressed libraries, so libraries are set to "false".
It's not only about a warning, but ajax is not working as the ajax call produces an error.

I'd say this is Major but "Cause a significant admin- or developer-facing bug with no workaround." it's pretty subjective so leaving it as Normal for now.

I think this is Major based on "Block contributed projects with no workaround.". From what we could see, the only way tot fix this was to patch core and roll back a fairly significant change.

+1 from me, I can see this in logs D 10.2.3

Regarding comment #5, which specific change did you roll back? What is the issue number? Is it definitely [#3389367]?

@cilefen, yes, that one.

@penyaskito do you mean that core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php was decompressing an already decompressed query parameter?

That should only be possible if it was running in a subrequest I think, but it only runs that logic on main request already. So then I wonder if something is somehow calling the URL l with the query parameter uncompressed. Can you post what the URL looks like for the actual AJAX request?

I've pushed up a commit with a possible workaround - if we fail to uncompress the query parameter, then don't replace ajax_page_state, that means that if it's completely invalid, garbage in garbage out, but if it's an uncompressed ajax_page_state, it ought to work.

If this works, maybe we can add this handling to UrlHelper::uncompressQueryParameter() directly instead - i.e. return the original string instead of FALSE.

Testing would be great because I'm not sure how to reproduce this.

It would still be good to understand the root cause of how we end up here in the first place. There are some related-ish issues like 🐛 Query string is appended multiple time after each AJAX request Needs work , 🐛 media_library_opener leads to massive GET requests that break varnish etc. Active 🐛 Pagination not working correctly in AJAX view with exposed filters Needs review with views AJAX and URL handling - the issues predate the ajax_page_state compression but they're interacting with it in a much more obvious way in combination with switching from POST to GET.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Need to update a unit test but that's for the specific behaviour we're changing here so can just change the assertion.

@catch Before testing the MR, I tested just removing our revert and it worked this time. We had video proof and detailed steps to reproduce (this hit production), so I'm pretty sure I'm testing it correctly.

Luckily we also logged the urls at that time. Not going to share here to avoid any leakages, but it had page_state[libraries] uncompressed.
If I repeat the same operations now I get a compressed page_state[libraries]

We clear caches on our sync process, so I'd bet it's not coming from caching.

You mentioned 🐛 Pagination not working correctly in AJAX view with exposed filters Needs review , and it's our case. This happened with inline entity browsers that embedded views where ajax paging and exposed filters were enabled. Both paging or trying to filter by a search term failed (independently, not necessarily combined).

I've looked at your MR and even if I can't reproduce now, I think it's still worth to be on the safe side and merge it.

After a second read at our issue tracker: not everyone could reproduce this locally (even with prod settings -redis, enforcing caching- enabled), and we could only reproduce in production and not on test environments with the same settings. So tbf I might be wrong on my bet it wasn't caching after all.

I think this makes everything safer, has test coverage for the new behavior, so RTBC.

Tests seem to be failing for something related.

Previously because we were only validating whether the string was compressed correctly, the test would pass because uncompressQueryParameter() returns false. Now if there is a completely invalid string, we need to validate a bit more closely. You could get the same PHP error if you compressed an incorrect library string and sent it as the parameter so this is good to tighten up more anyway. Actual invalid library definitions are handled by the API already, it's just strings that aren't library definitions at all which it doesn't like.

leftover comment, possibly when the method became an anonymous function

Oops sorry, yeah I got that far then realised why am I adding a method for something called from within the same method in two places, anonymous functions keep the logic closer together. Removed that.

Re-titled so title is up-to-date with what we are actually doing here.

As I couldn't reproduce this anymore (#15), I at least verified that everything still worked with this applied (had to apply unreleased 🐛 Skip query string compression if zlib extension isn't available Fixed before so it applied), and it works.

I would RTBC, but there's a phpcs complain. Just left a suggestion.

Just committed myself, so RTBC if green.

Couple of nits in the MR.

@penyaskito fixed the comment, and I replied on the MR about moving to a method on AttachedAssets.

Fair point about the exception; opened 📌 AttachedAssets should validate libraries internally Active as a followup.

Committed and pushed bfaae1b138 to 11.x and 895a6a913f to 10.3.x and 596d2c097c to 10.2.x. Thanks!

FYI: This broke asset injector ( 🐛 10.2.6 Error - CSS preprocessing / aggregation fails, resulting in broken page / style RTBC ) with Exception:

The libraries to include are encoded incorrectly

which was hard to find.

It broke because asset_injector used a / in the library name: https://git.drupalcode.org/project/asset_injector/-/merge_requests/16/diffs so "encoding" seems misleading here?
(I would have thought of UTF-8 vs. ISO etc... ;) )

Here a problem reported in Durpal Core 10.2.6:
https://www.drupal.org/forum/support/upgrading-drupal/2024-05-02/upgradi... →

@Anybody

        if (substr_count($library, '/') !== 1) {
          throw new BadRequestHttpException('The libraries to include are encoded incorrectly.');

Should we change that check to === 0? (and improve the exception message)

If so let's open a followup.

@longwave: Looks like a good plan and improvement to me, but I have to admit I'm not deep enough into this, so perhaps someone should take a look who was part of the implementation here.

I'll open a follow-up anyway.

Here's the follow-up: 🐛 Improve "The libraries to include are encoded incorrectly" check and message Active

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