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.
Comment | File | Size | Author |
---|---|---|---|
#44 | contact_storage-action_check-2834490-44.patch | 550 bytes | whiz11 |
#31 | interdiff-2834490-28-31.txt | 718 bytes | katherined |
#31 | contact_storage-action-hook-2834490-31.patch | 11.8 KB | katherined |
|
Comments
Comment #2
BerdirI 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.
Comment #3
BerdirAh sorry, this is not update but install. Yeah, I guess you have the message module installed?
Comment #4
samuel.mortensonI 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.
Comment #5
samuel.mortensonHere'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.
Comment #6
samuel.mortensonCleaned up the update hook a bit.
Comment #7
andypostI think that should be hook_post_update_NAME()
does it makes sense to keep uuid of the old action for config sync?
Comment #8
jibranNW for #7.]
> does it makes sense to keep uuid of the old action for config sync?
Yes.
Comment #9
Carlos Miranda Levy CreditAttribution: Carlos Miranda Levy as a volunteer commentedThis 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).
Comment #10
sajosh CreditAttribution: sajosh commentedPatch #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?
Comment #11
quixxel CreditAttribution: quixxel commentedAny progress on this?
I'm also unable to install contact storage if messages is installed.
Comment #12
webcaetera CreditAttribution: webcaetera commentedI can't install contact storage for this reason...
2 years later...
There is no adapted version of this module ?
Comment #13
larowlanThe 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
Comment #14
andypostComment #15
andypostNeeds rework on top of core new action
Comment #16
katherinedTaking a stab at this.
Comment #17
katherineda new stab.
Comment #18
phenaproximaHonestly, this is looking pretty good overall. Only a couple of small complaints from me. Nice work, @katherined!
I would remove "...for the DeleteMessage action plugin", since it no longer exists. :)
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."
This should use the ConfigEntityUpdater pattern from core. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... for documentation...
Comment #19
andypostInstead of that better to use ConfigUpdater like core using https://git.drupalcode.org/project/drupal/commit/281c6f0923432fb7bf70cfd...
Comment #20
BerdirHm, 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?
Comment #21
katherinedMore changes, including replacing the DeleteMultiple class.
Comment #22
katherinedComment #24
Berdirthis 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.
I would expect that \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider::getRoutes() generates this route automatically?
this seems to match he default implementation.
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.
this change looks unrelated and is probably the reason for the failing test?
Comment #25
katherinedI have no idea about #5. Rogue delete fingers with a mind of their own... lol
Comment #26
phenaproximaI 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 :)
Comment #27
BerdirI do :)
Error: Class 'Action' not found in modules/contact_storage/contact_storage.post_update.php on line 12
Missing use.
Comment #28
katherinedComment #29
katherinedComment #30
phenaproximaThis 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:
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.
Comment #31
katherinedComment #32
phenaproximaMagnifico!
Nit: This should end with a period. Fixable on commit.
Because $definitions itself is a reference, we don't need the ampersand on this line. Fixable on commit.
Comment #33
larowlanshould we have an update test for this? #27 might indicate so?
Comment #34
BerdirI'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.
Comment #35
andypostUpgrade tests are slow, so no reason for it.
Meantime makes sense to check that expected action exists after install
Comment #36
andypostOne more nitpick - better to use
DeleteMultipleForm:class
Comment #37
phenaproximaCan 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.Comment #39
BerdirFixed #32 on commit. Having regular tests of the delete operation would be great, but we didn't have any before either.
Comment #41
saltednutWe 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.
Comment #42
aangel CreditAttribution: aangel at Performant Labs commentedHi, 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.
Comment #43
larowlanI raised the above with @Berdir who said
Comment #44
whiz11 CreditAttribution: whiz11 commentedI 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.