Hi,

feel free to downgrade the risk of this issue, but I found conflicts implementing Contact Storage on existing installation.

The error is 'Unable to install Contact storage, system.action.message_delete_action already exists in active configuration.' using drush en command or admin interface.

This happens because Contact Storage has an install configuration file (system.action.message_delete_action.yml) which defines the DeleteMessage action in src/Plugin/Action.

The system.action.message_delete_action is already defined by other modules which perform different functionalities. I did not found any documentation to solve this kind of issues.

Technically this involves any D8 version of the module, but I'm pretty new to Drupal, so maybe this can be a limitation by design.

Thank you in advance,
D.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DavideDev created an issue. See original summary.

Berdir’s picture

I believe the latest dev already makes the installation of that optiona. But yes, I guess the name is bad, should be contact_message or so.

Berdir’s picture

Ah sorry, this is not update but install. Yeah, I guess you have the message module installed?

samuel.mortenson’s picture

I use the Message module and have also run into this - I agree with #2 in that we should prefix that action with something unique like contact_message.

samuel.mortenson’s picture

Status: Active » Needs review
FileSize
3.8 KB

Here's a patch which renames the action entity and the action plugin, which both conflict with the Message module. I've also included an update hook for existing users, which seems to work from my testing.

samuel.mortenson’s picture

Cleaned up the update hook a bit.

andypost’s picture

+++ b/contact_storage.install
@@ -120,3 +120,32 @@ function contact_storage_update_8200() {
+function contact_storage_update_8202() {
...
+  \Drupal::service('plugin.manager.action')->clearCachedDefinitions();
...
+  if ($action && $action->getType() === 'contact_message') {
+    $action->delete();
...
+  if (!$action_storage->load('contact_message_delete_action')) {
...
+    $storage = new FileStorage($config_install_path);
+    $action_storage
+      ->create($storage->read('system.action.contact_message_delete_action'))
+      ->save();

I think that should be hook_post_update_NAME()

does it makes sense to keep uuid of the old action for config sync?

jibran’s picture

Version: 8.x-1.0-beta8 » 8.x-1.x-dev
Status: Needs review » Needs work

NW for #7.]

> does it makes sense to keep uuid of the old action for config sync?
Yes.

Carlos Miranda Levy’s picture

This remains a problem when trying to install the module on sites with message module installed (and Distributions that use message by default as Open Social).

sajosh’s picture

Patch #6 seems to work for me. I have Message modules enabled.

drupal 8.4.3 upgraded from D6
PHP Version 7.1.12
Memory limit 524M
Database Version 5.5.58-0+deb7u1-log

What needs work? (#8) Should I not use this patch?

Do you want me to test anything?

quixxel’s picture

Any progress on this?
I'm also unable to install contact storage if messages is installed.

webcaetera’s picture

I can't install contact storage for this reason...
2 years later...
There is no adapted version of this module ?

larowlan’s picture

The patch needs work for #7. Running this module with message module is obviously not a common problem, else someone would have finalized the patch.

If you are experiencing the issue, perhaps keep things moving by making the changes suggested in #7

andypost’s picture

andypost’s picture

katherined’s picture

Taking a stab at this.

katherined’s picture

phenaproxima’s picture

Honestly, this is looking pretty good overall. Only a couple of small complaints from me. Nice work, @katherined!

  1. +++ b/contact_storage.module
    @@ -510,3 +510,10 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +/**
    + * Implements hook_action_info_alter() for the DeleteMessage action plugin.
    + */
    

    I would remove "...for the DeleteMessage action plugin", since it no longer exists. :)

  2. +++ b/contact_storage.module
    @@ -510,3 +510,10 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +  $definitions['message_delete_action'] = &$definitions['entity:delete_action:contact_message'];
    

    A comment above this might be useful. Something like "For backwards compatibility, treat 'message_delete_action' as an alias of the 'entity:delete_action:contact_message' plugin provided by core."

  3. +++ b/contact_storage.post_update.php
    @@ -0,0 +1,38 @@
    +/**
    + * Renames the "message_delete" action to avoid Message module conflicts.
    + */
    +function contact_storage_post_update_rename_message_delete_action() {
    +  // Clear Action plugin cache.
    +  \Drupal::service('plugin.manager.action')->clearCachedDefinitions();
    +
    +  $action_storage = \Drupal::entityTypeManager()->getStorage('action');
    +
    +  /** @var \Drupal\system\Entity\Action $action */
    +  $action = $action_storage->load('message_delete_action');
    +
    +  // Delete the old action, if it exists.
    +  if ($action && $action->getType() === 'contact_message') {
    +    $action->delete();
    +  }
    +
    +  // Create the new action, unless it already exists.
    +  if (!$action_storage->load('contact_message_delete_action')) {
    +    $config_install_path = \Drupal::moduleHandler()
    +        ->getModule('contact_storage')
    +        ->getPath() . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
    +    $storage = new FileStorage($config_install_path);
    +    $action_storage
    +      ->create($storage->read('system.action.contact_message_delete_action'))
    +      ->save();
    +  }
    

    This should use the ConfigEntityUpdater pattern from core. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... for documentation...

andypost’s picture

+++ b/contact_storage.post_update.php
@@ -0,0 +1,38 @@
+function contact_storage_post_update_rename_message_delete_action() {
+  // Clear Action plugin cache.
+  \Drupal::service('plugin.manager.action')->clearCachedDefinitions();
+
+  $action_storage = \Drupal::entityTypeManager()->getStorage('action');
+
+  /** @var \Drupal\system\Entity\Action $action */
+  $action = $action_storage->load('message_delete_action');
+
+  // Delete the old action, if it exists.
+  if ($action && $action->getType() === 'contact_message') {
+    $action->delete();
+  }
+
+  // Create the new action, unless it already exists.
+  if (!$action_storage->load('contact_message_delete_action')) {
+    $config_install_path = \Drupal::moduleHandler()
+        ->getModule('contact_storage')
+        ->getPath() . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
+    $storage = new FileStorage($config_install_path);
+    $action_storage
+      ->create($storage->read('system.action.contact_message_delete_action'))
+      ->save();
+  }

Instead of that better to use ConfigUpdater like core using https://git.drupalcode.org/project/drupal/commit/281c6f0923432fb7bf70cfd...

Berdir’s picture

Hm, I'm not sure that using ConfigUpdater here helps much, at least not with the current implementation.

It's purpose is to apply a single change to multiple config entities, handling batch and saving as necesasry, but we only update a single one here.

That said, I would suggest we try to rename the one we have instead of deleting and re-creating it. Creating new config during update functions is tricky as messes with being able to use config export/import of the result as each run on dev/staging/prod results in a different UUID. Technically, ConfigUpdater would work then, but I doubt it makes things easier?

katherined’s picture

More changes, including replacing the DeleteMultiple class.

katherined’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: contact_storage-action-hook-2834490-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

  1. +++ b/contact_storage.post_update.php
    @@ -14,25 +14,3 @@
    +  \Drupal::configFactory()->rename('system.action.message_delete_action', 'system.action.contact_message_delete_action');
    

    this doesn't rename the id inside the file, which is going to confuse the system.

    To rename a config entity, you should load it, change the id and then save, see \Drupal\KernelTests\Core\Config\ConfigCRUDTest::testCRUD() for an example.

    And, we also need to update the plugin key.

  2. +++ b/contact_storage.routing.yml
    @@ -25,6 +25,15 @@ entity.contact_form.enable:
    +
    +entity.contact_message.delete_multiple_form:
    +  path: '/admin/structure/contact/messages/delete'
    +  defaults:
    +    _form: '\Drupal\contact_storage\Form\ConfirmDeleteMultiple'
    

    I would expect that \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider::getRoutes() generates this route automatically?

  3. +++ b/src/Form/ConfirmDeleteMultiple.php
    @@ -0,0 +1,38 @@
    +   * {@inheritdoc}
    +   */
    +  public function getCancelUrl() {
    +    return new Url('entity.contact_message.collection');
    +  }
    

    this seems to match he default implementation.

  4. +++ b/src/Form/ConfirmDeleteMultiple.php
    @@ -0,0 +1,38 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function getDeletedMessage($count) {
    +    return $this->formatPlural($count, 'Deleted @count message.', 'Deleted @count messages.');
    +  }
    

    the default implementation of these forms use items instead of messages, but IMHO, that's good enough for us, so I'd just use the default implementation of this class directly.

  5. +++ b/contact_storage.module
    @@ -337,8 +337,9 @@ function contact_storage_entity_type_alter(array &$entity_types) {
       // Add clone link for forms.
    -  $entity_types['contact_form']->setLinkTemplate('clone-form', '/admin/structure/contact/manage/{contact_form}/clone');
    +  $entity_types['contact_form']->setLinkTemplate('clone-form', '/admin/structure/contact/manage/clone');
     
    

    this change looks unrelated and is probably the reason for the failing test?

katherined’s picture

I have no idea about #5. Rogue delete fingers with a mind of their own... lol

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/contact_storage.module
@@ -510,3 +512,11 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
+ * For backwards compatibility, treat 'message_delete_action' as an alias of the 'entity:delete_action:contact_message' plugin provided by core.

I think this should be a // comment in the body of the function, above the line of code. Also, small coding standards thing: each line of a comment should not exceed 80 characters in width, so this needs to be formatted a bit. But honestly...that's probably fixable on commit.

I see nothing else wrong here. RTBC once it passes tests :)

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

I do :)

Error: Class 'Action' not found in modules/contact_storage/contact_storage.post_update.php on line 12

Missing use.

katherined’s picture

katherined’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work
+++ b/contact_storage.module
@@ -515,8 +515,9 @@
+//For backwards compatibility, treat 'message_delete_action' as an alias of
+// the 'entity:delete_action:contact_message' plugin provided by core.

This isn't quite there -- this needs to be inside the function. (Inline comments, like these, are usually inside functions; doc comments, in the /* */ form, are usually above them.) So something like this:

/**
 * Implements hook_action_info_alter().
 */
function contact_storage_action_info_alter(array &$definitions) {
  // For backwards compatibility...
}

Additionally, I noticed two small things, one of which is important. First, inline comments need a space after the //, for every line. (That's the less important thing. :)

Secondly, the $definitions parameter in this function needs to be a reference -- so it should be &$definitions. This is actually super important, because without it, the alias is not going to be "picked up" by the system and there could be backwards compatibility breaks.

katherined’s picture

Status: Needs work » Needs review
FileSize
11.8 KB
718 bytes
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Magnifico!

  1. +++ b/contact_storage.module
    @@ -510,3 +512,12 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    + * Implements hook_action_info_alter()
    

    Nit: This should end with a period. Fixable on commit.

  2. +++ b/contact_storage.module
    @@ -510,3 +512,12 @@ function contact_storage_contact_form_delete(EntityInterface $entity) {
    +  $definitions['message_delete_action'] = &$definitions['entity:delete_action:contact_message'];
    

    Because $definitions itself is a reference, we don't need the ampersand on this line. Fixable on commit.

larowlan’s picture

+++ b/contact_storage.post_update.php
@@ -0,0 +1,20 @@
+function contact_storage_post_update_rename_message_delete_action() {

should we have an update test for this? #27 might indicate so?

Berdir’s picture

I've tested it now successfully manually,

I'd rather not hold this up on an upgrade test, which honestly is a pain to create for a contrib module, either we need to create a complete dump and have that in the module or set up everything by hand based on a core dump in SQL statements, which is really hard.

Will give you some time to disagree with that before committing.

andypost’s picture

Upgrade tests are slow, so no reason for it.
Meantime makes sense to check that expected action exists after install

andypost’s picture

+++ b/contact_storage.module
@@ -330,6 +330,7 @@ function contact_storage_entity_type_alter(array &$entity_types) {
+  $entity_types['contact_message']->setFormClass('delete-multiple-confirm', 'Drupal\Core\Entity\Form\DeleteMultipleForm');

One more nitpick - better to use DeleteMultipleForm:class

phenaproxima’s picture

One more nitpick - better to use DeleteMultipleForm:class

Can we do this in a follow-up issue? There are other pre-existing class references in contact_storage_entity_type_alter() which are not using the ::class syntax; it seems preferable to fix them all at once.

  • Berdir committed afaef1e on 8.x-1.x authored by katherined
    Issue #2834490 by katherined, samuel.mortenson, Berdir, phenaproxima:...
Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Fixed #32 on commit. Having regular tests of the delete operation would be great, but we didn't have any before either.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

saltednut’s picture

We last saw a release for this module in may even though this and two other issues have been fixed since. Its never fun to post these kinds of request, but kindly... what needs to be done to get this into another beta?

As always, if there's release blocking issues, happy to discuss how to fix them too. Thanks for understanding the nature of this comment.

aangel’s picture

Hi, following up on saltednut's comment, would it be possible to get a release with this fix in it? It's not possible to install the module on sites with Message installed, such as Open Social sites.

How can I help?

Cheers.

larowlan’s picture

I raised the above with @Berdir who said

the commit in that issue was in 2019 and has been in tagged releases for a long time

whiz11’s picture

Version: 8.x-1.x-dev » 8.x-1.2
FileSize
550 bytes

I was having an issue when performing an upgrade from 8.4 on a project. Turns out the naming still had an issue. Attaching a patch.