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