Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When deleting bundles (eg. node types), their language configuration is not deleted.
Proposed resolution
Make sure they are deleted and add tests.
Remaining tasks
Review. Commit.
User interface changes
No.
API changes
No.
Comments
Comment #1
kfritscheProposal
Everything in the configuration will be deleted, when a bundle is deleted, so this should be deleted to.
We already have a language_node_type_update in language, to detect changes in the bundle name, so it could be easily added a delete hook to. Attached a inital patch for this.
I wonder why a node_type_update is used and not a entity_bundle_rename? So only when node bundles are changed the bundle keeps the translation settings. In the VocabularyFormController is a special submit function to take care of that, which could be removed. Further comment bundles are currently not renamed at all. Didn't changed it yet, as I'm not sure of the impact.
Also I noticed while testing, when creating a new bundle a entry in language.settings.yml is added, with no bundle name. In the element process function is the old value and not the new one. Added TODO in attached patch.
Added a rename and delete entity_bundle function for translation_entity too, as there its completly missing. Here the problem is that after renaming is done, somewhere the translation data are set again, with the old bundle name. Didn't figured out yet, where this is happening.
Comment #2
YesCT CreditAttribution: YesCT commentedNote, when working on #1945226: Add language selector on menus I also got an empty machine name saved in language settings. That's when I figured out what the submit handler was for that was in the taxonomy example I was basing it off of.
See #51 and #59
I dont know if something generic can be added to language, so that every entity does not need to have this custom thing.
Comment #3
YesCT CreditAttribution: YesCT commentedopened #2015615: Comment bundles, other newly languagifiied entities get empty machine in language.settings.yml without custom submit handler
need to clarify (on that issue is fine) what are the steps to reproduce the empty machine name in language settings? Was it for comments? How did you make a new comment bundle?
Comment #4
YesCT CreditAttribution: YesCT commentedadding the link to the todo issue, and change https://drupal.org/node/1354#inheritdoc (and copy and paste which referenced delete for the rename inherit) and used https://drupal.org/node/1354#todo format for @todo
Comment #5
Gábor HojtsyThe meat of the test is commented out, so not sure it proves anything (yet). Comments on the code:
I don't think we need to mention this here, we have an issue for this one.
How can procedural functions have inheritdoc? Where would they inherit from?!
Comment #6
YesCT CreditAttribution: YesCT commentedoops. hooks!
Comment #7
YesCT CreditAttribution: YesCT commentedSorry about that, hooks dont use inheritdoc!
https://drupal.org/node/1354#functions
Comment #8
YesCT CreditAttribution: YesCT commentedOops. I think I accidentally included the test from #1945226: Add language selector on menus when I made one of the patches above.
That needs to be taken out of this patch.
I think a test here should be a test on the test Enitity.
Comment #9
YesCT CreditAttribution: YesCT commentedI'll work on this.
Comment #10
YesCT CreditAttribution: YesCT commentedJust undoing stuff I messed up since #1.
so change since #1, is just adding a link to the issue the todo is for.
@Gábor Hojtsy throught we might be able to just delete the todo since there is an issue, but I thought we kept them in the code too sometimes.
I dont mind either way.
The only other change from #1 is reverting the permissions on .module
After this, I'll try and add a test.
Comment #11
YesCT CreditAttribution: YesCT commentedmanually testing this patch, I find it fixes this other problem #1739928-176: Generalize language configuration on content types to apply to terms and other entities that results from changing the machine name.
Comment #13
YesCT CreditAttribution: YesCT commentedI didn't get very far on this today. Taking a break, so someone else can work on the test.
This fixes the hook name I noticed earlier (but got lost in the inheritdoc debacle) .
Does the location for this test seem reasonable?
Note the test doesn't work. I'm not sure how to "delete" the bundle. I'm not even sure if that bundle was created, or if it's just a name being used to set the config stuff.
Comment #14
BerdirAs this entity type does not actually exist and you can't delete it for real, you need to invoke the hook that would be invoked if the bundle would be deleted directly:
entity_invoke_bundle_hook('delete', 'some_entity_type', 'some_bundle') should do the trick.
Comment #15
jair CreditAttribution: jair commentedComment #16
balintcsaba CreditAttribution: balintcsaba commentedComment #17
balintcsaba CreditAttribution: balintcsaba commentedRerolled.
Comment #19
balintcsaba CreditAttribution: balintcsaba commentedrerolled
Comment #21
mgiffordComment #22
stefank CreditAttribution: stefank commentedIm at DrupalCon Amsterdam 2014 and working on it.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #24
kfritscheRerolled.
Finished the test which was started but didn't worked yet.
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedIssue summary "Proposed resolutions" needs update.
Comment #29
andypostI think the scope is too small, this require CUD covered or at least issue for update needed
so update supported only for node types?
Comment #30
penyaskitoPlease see also #2355909: language.settings config is not scalable.
Comment #31
Gábor HojtsyPostponing on #2355909: language.settings config is not scalable then. That should provide full CRUD but tests that it happens on entity bundle actions will still be useful unless already added there.
Comment #32
Gábor HojtsyComment #33
Gábor HojtsyDid #2355909: language.settings config is not scalable resolve this entirely?
Comment #34
penyaskitoYep, we covered this, but we miss test coverage for deletion (we have for addition and update).
We should reuse the test (and update the IS).
Comment #35
Gábor HojtsyRetitled for what needs to be done. Updated issue summary.
Comment #36
penyaskitoI'm working on this one in context of the Global Sprint Weekend 2015.
Comment #37
DevElCuy CreditAttribution: DevElCuy commentedComment #38
DevElCuy CreditAttribution: DevElCuy commentedRemoved tag by mistake.
Comment #39
penyaskitoMmmmm... interesting.
The test actually reveals that this is still a bug on HEAD. But I expected this to be solved by the configuration dependencies. What's wrong there? The proposed fix may not be the best way, as IMHO this should be handled at the config dependency level.
Comment #41
alexpottThere is nothing automatic about deleting a config entity that is a dependency of another config entity and having that config entity deleted. The only place where something like this occurs in extension uninstall.
This should only do the delete if it needs to. I've never really liked the fact that the load creates new object. Feels icky.
Comment #42
BerdirWell, not creating the object would mean that we would need 5 lines of code instead of 2 everywhere where we use it. But yes, should do an !isNew() check.
Comment #43
Gábor HojtsyRetitled according to the current status. Also updated issue summary.
Comment #44
penyaskitoChecked that the entity existed before deleting.
Comment #45
BerdirI think #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted should take care of this automatically, we're finally at a place to automatically act based on the dependency information that we have.
Comment #50
penyaskito@Berdir: that's awesome! I missed that one.
However IMHO getting this in still is interesting because of the additional tests.
Comment #52
penyaskitoWrong order of the patch uploads, oops.
Comment #53
Gábor HojtsyI agree additional test coverage would be useful even if the other one fixes the issue. Only found English issues.
settings
Remove "are" IMHO, I think you meant "our" but its fine without it.
Remove the second "the".
"article" is the bundle name.
has been?
(given its one file).
was instead of were for both no?
Comment #54
penyaskitoFixed grammar.
Comment #55
Gábor HojtsyAll righto, thanks!
Comment #56
Gábor HojtsyWait, patch includes unrelated things.
Comment #57
penyaskitoOops, wrong patch. Interdiff was right.
Comment #58
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 6cfaed4 and pushed to 8.0.x. Thanks!
Comment #60
Gábor HojtsyYay, thanks!