Convert Redirect entity to revisionable

Adding an update hook to resolve the entity mismatch error on the rid field.

Fixed wrong patch posted in comment #18.

I'd like to see what tests say, marking NR.

1. +++ b/redirect.install
   @@ -236,3 +236,29 @@ function redirect_update_8109() {
   +function redirect_update_8110() {
   
   +++ b/redirect.post_update.php
   @@ -0,0 +1,124 @@
   + * Add the redirect revision table.
   

   Should this be moved to redirect_post_update_make_redirect_revisionable?

2. +++ b/src/Entity/Redirect.php
   @@ -95,26 +113,6 @@ class Redirect extends ContentEntityBase {
   -  public function setLanguage($language) {
   ...
   -  public function setStatusCode($status_code) {
   

   I don't see why deleting these functions and adding the setDynamic. Aside of this breaking APIs, it just doesn't make sense to me.

The last submitted patch, 14: 3313540-14.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 15: 3313540-15.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 16: 3313540-16.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 19: 3313540-19.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Implemented the suggestions from #20:

• Moved the schema update into the post update hook.
• Removed setRedirectDynamic and restored the functions it replaced.

I'm not convinced about this. I understand that a few people have this use case, but I maintain sites with tens and hundreds of thousands of redirects, and this update would be very expensive for them, result in extra storage and it's not useful for 99% of the sites.

This seems non-intrusive enough that it could be done in a separate project for those that require it, by replacing the entity class and hooks.

@berdir, revision (and publishing status) support is also needed by Workspaces, because at the moment we only have the "ignore" approach from 📌 Allow CRUD operations for Redirect entities in a workspace Needs review , which basically means that any redirect created in a workspace is also available in Live, and that's not really a great (nor expected) behavior :)

For your concern about extra storage, I think we shouldn't create new revisions by default like the latest patch does, but allow this behavior to be configured for people that need it.

Started a MR from a reroll of the patch in #25.

Needs work to handle redirect_delete_by_path() somehow..

The patch on #25 is not applying on 1.11, so just making a reroll to apply correctly

The MR is ready now :)

Here's a patch from the current MR that applies on top of ✨ Add back 'disabled' redirects Active , for folks that need Workspaces support :)

Postponing on ✨ Add back 'disabled' redirects Active so the upgrade path doesn't clash. This MR uses redirect_update_8111() and the other issue uses redirect_update_8110().

Blocker is in!

Added a a comment

Brought back the optimized query, and didn't inject anything for maximum performance :)

Hm, we still always call it, so it doesn't really make a difference if it's injection or not. A conditional injection would avoid the module handler.

See 📌 Explore using more service closures to break deep dependency chains and load fewer services Active on the script I use to evaluate when things are instantiated. Maybe it doesn't make difference if that's already used at that point?

Updated the MR to inject the workspace manager conditionally because the code is cleaner, but it doesn't make a difference indeed :)

Using the snippet from 📌 Explore using more service closures to break deep dependency chains and load fewer services Active , here's the dependency chain where redirect.repository appears when Workspaces is not installed:

redirect.request_subscriber -> redirect.repository -> entity_type.manager -> string_translation -> string_translator.custom_strings -> entity.last_installed_schema.repository -> redirect.checker -> access_manager -> paramconverter_manager -> drupal.proxy_original_service.paramconverter.views_ui -> tempstore.shared -> keyvalue.expirable -> router.admin_context -> current_route_match -> entity.repository -> context.repository -> drupal.proxy_original_service.paramconverter.configentity_admin -> paramconverter.entity -> paramconverter.entity_revision -> drupal.proxy_original_service.paramconverter.menu_link -> plugin.manager.menu.link -> Drupal\Core\Menu\MenuTreeStorageInterface -> menu_link.static.overrides -> drupal.proxy_original_service.node_preview -> tempstore.private -> access_arguments_resolver_factory -> Drupal\Core\Access\CheckProviderInterface -> router.request_context

And when Workspaces is installed:

redirect.request_subscriber -> redirect.repository -> redirect.checker -> access_manager -> paramconverter_manager -> drupal.proxy_original_service.paramconverter.views_ui -> tempstore.shared -> keyvalue.expirable -> router.admin_context -> current_route_match -> Drupal\Core\Entity\EntityRepositoryInterface -> context.repository -> drupal.proxy_original_service.paramconverter.configentity_admin -> paramconverter.entity -> paramconverter.entity_revision -> drupal.proxy_original_service.paramconverter.menu_link -> plugin.manager.menu.link -> Drupal\Core\Menu\MenuTreeStorageInterface -> menu_link.static.overrides -> drupal.proxy_original_service.node_preview -> tempstore.private -> access_arguments_resolver_factory -> Drupal\Core\Access\CheckProviderInterface -> router.request_context

FWIW, the workspace manager is already instantiated much earlier in the request, here:

options_request_listener -> router.route_provider -> state -> keyvalue -> cache.bootstrap -> cache.backend.chainedfast -> cache.backend.apcu -> lock -> keyvalue.database -> path.current -> cache.data -> path_processor_manager -> path_processor_decode -> path_processor.image_styles -> path_processor_front -> config.factory -> config.storage -> cache.config -> config.typed -> config.storage.schema -> cache.discovery -> validation.constraint -> container.namespaces -> path_processor.files -> path_alias.path_processor -> Drupal\path_alias\AliasManagerInterface -> path_alias.repository -> workspaces.manager -> entity_type.manager -> string_translation -> language.default -> string_translator.custom_strings -> entity.last_installed_schema.repository -> entity.memory_cache -> current_user -> logger.channel.workspaces -> logger.factory -> logger.dblog -> logger.log_message_parser -> logger.drupaltodrush -> workspaces.association -> workspaces.repository -> cache.default -> workspaces.information -> workspaces.negotiator.query_parameter -> workspaces.negotiator.session -> path_alias.prefix_list -> language_manager -> cache_tags.invalidator -> cache.static -> cache.backend.memory -> cache.entity -> cache.menu -> cache.render -> cache.access_policy -> cache.dynamic_page_cache -> cache.toolbar -> cache.access_policy_memory -> cache.backend.memory.memory

This will need another rebase due to DI changes.

All done :)

Added some review questions. I very concerned about the hash value and \Drupal\redirect\Entity\Redirect::preSave()

Thanks @alexpott for the review, very good catch for the hash field! I fixed all points and added test coverage :)

I think we just missing test coverage of the workspace version of finding by hash and then we're good. Maybe we need to look for more queries like the one in \Drupal\redirect\RedirectRepository::findMatchingRedirect() - i.e. a direct SQL query on the redirect table.

I'm wondering about how Redirect's submodules play with this. For example, should the 404 recorder be recording when in a workspace... I guess that's not really a revisionable issue... but we should at least have a think and check to see if there is an issue open already.

Added test coverage for findMatchingRedirect() (and implicitly for findByHash()) by reusing an existing test method and running it inside a workspace.

For example, should the 404 recorder be recording when in a workspace

I think it should, for example content editors can delete path aliases in a workspace and create redirects for them, and it would be good to notice any mistakes in this process.

I think it should, for example content editors can delete path aliases in a workspace and create redirects for them, and it would be good to notice any mistakes in this process.

But what happens if those 404s are only 404s in the workspace... they'll show up on the report even when a user is not in the workspace.

But what happens if those 404s are only 404s in the workspace... they'll show up on the report even when a user is not in the workspace.

Right, and I don't see that as problematic. Content editors are well aware that the site is using workspaces, and very likely knowledgeable about what content edits are done in each workspace.