Sort Get parameters

Retrying the tests against 2.1.x - need to get green before this can land.

Makes perfect sense to me.

I think all this needs is an empty post-update hook to ensure caches are flushed, as if any formatter output is held in caches before this change, there will be hash mismatches until it's invalidated.

This can easily be back ported to 2.0.x, as 2.1.x isn't ready for prime time yet.

Err.. actually shouldn't the sorting happen in the ::getHash method?

Hi,

Thanks for the review. MR updated.

Also updating the version in the issue as the MR target branch had been updated.

I’m leaving here a patch that replicates the changes proposed in the MR but is compatible with version 2.0.5, in case it might be useful for someone. In our project, we are not allowed to deploy development versions, and we needed to have this resolved with the current stable version. You can ignore this patch when it comes to solving the issue.

This patch solved the exact situation described in the issue: a site behind a proxy that was sorting the query params, and this module was returning access denied.

Moving to a bug report as the module should just check the integrity of the params, not the ordering.

I suspect the errors reported by the CI are not related to this MR, and this why I not moving it to RTBC, but I would say it is.

It's my understanding that the mere presence of a post-update hook is sufficient to ensure that caches are flushed appropriately. I don't think we need to call the cache flushing function manually inside.

Almost there for 1.x🙂.

Regarding the 2.x branch and tests, it looks like Drupal 11 has indeed broken some functionality that will need to be addressed 😞.

Best to stick with the stable release until that bag of worms can be resolved.

Thanks for the review (and the module!), Luke.

Regarding the post-update hook , you mean that:

/**
 * Flush cache to regenerate hash with sorted parameters.
 */
function oembed_lazy_load_post_update_regenerate_hash(): void {
}

would be enough? That would be the only change required, right?

Yep! That should do it. I'm not 100% positive on the rationality behind it, but it's a very common pattern for when changes to a module (or core) require a cache flush.

If I had to guess it's so that site owners will see that database updates are required in their dashboards, and when they run said updates, the caches naturally flush out.