EU = Entity Usage module
ET = Entity Track module

Currently, EU 4.x + ET 1.x = EU 2.x regarding features parity.
https://www.drupal.org/project/entity_usage/issues/3324787
https://www.drupal.org/project/entity_track/issues/3324797

But the source config has been moved to the entity_track module and we need an upgrade path for that.

CommentFileSizeAuthor
#6 3326110-6.patch1.73 KBseanb
#4 3326110-4.patch1.48 KBseanb

Issue fork entity_usage-3326110

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

askibinski created an issue. See original summary.

askibinski’s picture

askibinski commented

The 95% usecase are people upgrading EU to 4.x and installing the ET module. However, it's also possible that some people already had the ET module installed, in which case we probably do not want to change any ET settings.

EU:
Update hook:

  • copy source settings config to a PrivateTempStore
  • remove source settings config

ET:
Install hook:

  • if EU exists and has source settings config, use those
  • or- if the privously set EU PrivateTempStore exists, use that

The PrivateTempStore is necessary for any people using a typical CI which first does database updates and then config import (enabling ET). Other people might first enable ET and then run updates..

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

The PrivateTempStore is necessary for any people using a typical CI which first does database updates and then config import (enabling ET). Other people might first enable ET and then run updates..

I don't think that's strictly necessary. When we moved from media_entity to media in core, we created an update hook in media_entity that did all sorts of updates, including installing new modules, creating / moving config, cleaning things up, etc. I would imagine we could use a similar approach here.

About preserving current settings, I'm not 100% sure on that either... I guess it would depend on how much more complex it makes things. There are really a small number of sites using Entity Track, and strictly speaking, usually there is no need to guarantee BC between alphas, so we might be good with just a a fair warning on the release page? On the other hand, maybe all this can be handled in the EU update hook, which can detect if ET is already installed, and just skip copying things over in that case?

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Active » Needs review
StatusFileSize
new 3326110-4.patch1.48 KB

I think this could do the trick.

seanB opened merge request !41

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
StatusFileSize
new 3326110-6.patch1.73 KB

Updated the PR to never override entity track settings with empty entity usage settings (just in case someone already had entity track installed and the settings migrated by hand).

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented
Status: Needs review » Needs work

Since a few services moved from Entity Usage into Entity Track, we should at least document the necessary changes people need to perform in custom code.

I tried the patch on a client project, where a custom service declares dependency on some of EU old services, and got:

Running drush @self cache:clear plugin

In CheckExceptionOnInvalidReferenceBehaviorPass.php line 86:
                                                                                                                         
  The service "ga_helper.usage_helpers" has a dependency on a non-existent service "plugin.manager.entity_usage.track".

The issue is that we can't easily swap the plugin manager here with the ET plugin manager, since it won't be enabled until after the update hooks are triggered. One workaround would be to skip the initial cache/plugin clear, but I don't think that's a good idea, since we don't know what deployment workflows people are using out there. Any ideas on how to avoid this issue?

askibinski’s picture

askibinski commented

@marcoscano

We ran into this exact same issue. What we did was swap the services of the custom yml anyway, and also modify the existing services using a temprary alter in
ServiceProviderBase.

Something like this:

  /**
   * {@inheritdoc}
   */
  public function alter(ContainerBuilder $container): void {
    if (!$container->hasDefinition('entity_track.manager')) {
      $et_manager = new Definition('EntityTrackManager');
      $et_manager->setPublic(TRUE);
      $container->setDefinition('entity_track.manager', $et_manager);
    }
  }

Still looking for a better solution, but this is a hard problem. I agree that this at least needs documenting but I'm not sure if we can fix this automatically for everybody?

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

I guess entity usage could provide the alter if we properly document what this is used for. It's a bit of a hack/workaround for deployment workflows, but if this keeps sites from getting stuck in a state they can't really get out of, I guess it is acceptable. We could mark it for removal in a 5.x version or something.

marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
commented

Thanks for the feedback!
I think it's a good idea to try to ship with the hack/workaround. Can we try to incorporate it in this patch?
I also think this issue is a good place for us to document (either in the readme of the module or directly in this issue's description, and we can then later copy to the release notes page) the API changes from 2.x to 4.x (services that moved namespace, etc). Basically anything people would need to know if they wrote custom code relying on public APIs from 2.x, so they can check if their custom code needs to be updated.
Thanks!