πŸ‡ΊπŸ‡ΈUnited States @Les Lim

Account created on 3 October 2006, over 17 years ago
#

Merge Requests

Recent comments

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Update message about availability in Drupal core

πŸ‡ΊπŸ‡ΈUnited States Les Lim

One problem with the previous MR is the getRouteCollectionForRequest() call, which calls out to cache. The PreviewLinkEntityHooks class that contains it is not a service; it is instantiated new for every entity access check, for which there could be dozens on any given page. Our call to get the current route collection thus gets called new every time, even though the result is the same each time.

Converting the class into a service saves memory and allows us to statically cache the getRouteCollectionForRequest() result, cutting down substantially on queries.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Thanks for the report! This looks like a duplicate of πŸ› Deprecation error message Fixed , which was committed but not released. I'm putting out 1.0.0-alpha5 now.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Thanks! I've opted instead to prevent the function from being invoked for empty values.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Here's a patch that adds a "canonical_reroute" configuration in preview_link.settings. It defaults to TRUE, and is overrideable via normal means (i.e., settings.php).

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Whoops, here's a fixed patch file.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Found out that entity access might be determined and cached before hook_entity_embed_context_alter() has a chance to run, so we need to reset the entity type access cache.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Here's a new patch for the 2.1.x branch. I've removed the section that larowlan highlighted in #3, this is just checking that route entity has a preview link, but this is already implicitly true when we check that the route is a preview link route.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

@DieterHolvoet Thanks!

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Shoot, MR #17 was intended to open against 2.1.x, but it's trying to merge into 2.0.x and I can't figure out how to change the MR or create a new one. What am I missing?

πŸ‡ΊπŸ‡ΈUnited States Les Lim

MR #16 doesn't apply cleanly anymore, since the entity hook logic has been moved to a service class in both 2.x branches. Opened MR #17 as a reroll with minor refactoring for dependency injection.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Les Lim β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

The 2.x branch has the same issue regarding embedded entities as the 1.x branch. When on a Preview Link route and evaluating the access result of embedded entities, nothing in the result stack explicitly returns an AccessResultAllowed, so the embedded entity is not rendered.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Committed #2, new release coming shortly. Thanks for the review, @tyler36!

πŸ‡ΊπŸ‡ΈUnited States Les Lim

Yep! Got some time blocked out this weekend.

πŸ‡ΊπŸ‡ΈUnited States Les Lim

In our case, we're using Locale module, but the configuration in locale.settings.yml is not installed. Just looking through our git history, I'm not sure it ever has been.

This hadn't been causing us any problems that we knew of until the PHP 8.1 upgrade. During locale_requirements(), the function locale_translation_source_build() is called for each project and attempts to build a filepath to a local .po file for the project. To do this, it uses a default filename pattern that is defined in locale.settings.yml. If you don't have any locale.settings configuration, strtr() gets passed NULL instead of a filename pattern.

Not having any locale.settings is clearly a bug on our side, but it sounds like we're not the only ones this has happened to. I'm happy to resolve our bad config state, but should there be safeguards here to account for this edge case?

πŸ‡ΊπŸ‡ΈUnited States Les Lim

There are a couple things that need work on this issue:

1) Patch Drupal 8. If the problem does not exist in Drupal 8, update the issue summary to explain why this does not need to be fixed for D8.

2) The reason why the patches pass tests is because no tests exist to catch the original bug. Any patch that would be a candidate for committing to D7 will need to include a test that verifies that the bug exists and that the patch fixes it.

Production build 0.69.0 2024