Follow up for #2014955: Deleted bundles do not have their language configuration deleted
Problem/Motivation
If you create a new node type, block content, vocabulary or comment type you get a entry without a machine name in language.settings.yml and the setting is not saved when editing it later.
There might be more entities that need custom submit handlers added to prevent empty machine names in saved language.settings.yml. Currently done is node type, block content, vocabulary and comment type.
Steps to reproduce:
- Install D8
- Enable the Language Module
- Add a content type, setting the default language to "English" (not site default)
- View
sites/default/files/config_*/staging/language.settings.yml
(after exported with e.g. drush cex)- Expected result: it contains an entry for the new content type.
- Actual result: it doesn't.
- Edit the created content type and check if the default language is set to "English"
- Expected result: "English"
- Actual result: "Site default"
Proposed resolution
Identity which,
or add something so that that is not necessary.
Remaining tasks
Discuss if there is a better solution than using submit hooks, which needs to be added manully.
User interface changes
No.
API changes
TBD.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal.empty-machine-name.2015615.26.patch | 12.57 KB | kfritsche |
#7 | 2015615-7.patch | 6.42 KB | marthinal |
#12 | 2015615-12.patch | 6.09 KB | YesCT |
#12 | interdiff-2015615-7-12.txt | 2.34 KB | YesCT |
#13 | interdiff-2015615-12-13.txt | 3.71 KB | marthinal |
Comments
Comment #1
kfritscheSteps to reproduce:
1. Current/fresh installation with content translation enabled. OR Check sites/default/config_*/language.settings.yml - There shouldn't be a '' under node.
2. Create a new node bundle - test (admin/structure/types/add).
3. Check sites/default/config_*/language.settings.yml.
- There is no bundle test under node, but a '' with the default language configuration.
- There is no bundle comment_node_test under comment
4. Set test bundle to be translatable in Content language settings (admin/config/regional/content-language)
5. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test under node, but '' still exists.
- There is now the bundle comment_node_test under comment.
6. Rename the machine name (!) of the test bundle to test_new
7. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test_new under node, but '' still exists.
- There is still the bundle comment_node_test under comment and not comment_node_test_new.
8. Just save settings in Content language (admin/config/regional/content-language)
9. Check sites/default/config_*/language.settings.yml again.
- There is now the bundle test_new under node, but '' still exists.
- There is now the bundle comment_node_test under comment and comment_node_test_new.
Comment #2
marthinal CreditAttribution: marthinal commentedI'm using the patch from #2014955: Deleted bundles do not have their language configuration deleted by Karl and Cathy. I think here the problem is that we don't have the type(bundle name) when the form is built and we need it. Maybe it is a good idea to use the form_state values in this case?
The behavior looks correct.
Comment #4
marthinal CreditAttribution: marthinal commentedAdded submit handler to node (entity type). I was debugging on a minimal installation without comment module enabled, so probably we need to take a look at the comment module too. Firstly let's take a look at this patch. We are writing the same behavior for different entities (menu, taxonomy, nodes...) maybe it is possible to do this directly for the entities and only in one place (inside the submit handler at the language module (language_configuration_element_submit())). Tests that have been failing, now are green at my localhost and checking manually language.settings.yml the bundle (node type) seems to be added as expected and removed too. The patch is different so we do not need an interdiff in this case.
Comment #5
marthinal CreditAttribution: marthinal commentedBerdir, YesCT and I had a discussion about it.
Each time we are using the same method languageConfigurationSubmit() adding this:
$form_state['language']['language_configuration']['bundle'] = $form_state['values']['type'];
The id is different per entity (i.e. node use type, Vocabulary use vid, menu use id) and it is possible to obtain this id using the property "bundle_of". This property is not actually in all the entities so we can only use it with node, custom block, vocabulary and not with menu or comments.
So the first step is to fix this problem using the submit handler. Add a test per entity and when fixed we can start thinking about do this in a smart way using bundle_of.
Comment #6
marthinal CreditAttribution: marthinal commented4: 2015615-4.patch queued for re-testing.
Comment #7
marthinal CreditAttribution: marthinal commentedAttached test + patch.
So let's fix this bug and then open a new issue to remove the submit handler as explained at #5. Probably the best way is using a hook_entity_insert().
Comment #10
marthinal CreditAttribution: marthinal commented7: 2015615-7.patch queued for re-testing.
Comment #11
YesCT CreditAttribution: YesCT commentedI'm reviewing this now.
Comment #12
YesCT CreditAttribution: YesCT commentedWe will not need this @todo, that is a link to this issue.
Why do we set() and get() with the settings_key (with '.language.default_configuration',
but clear() uses the bundle_key instead?
nit, @file uses Contains now.
https://drupal.org/node/1354#file
fixed.
un-used use.
fixed.
I think this was cut and paste'd from another test.
nit: Fixed to start with third person verb.
"Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides..."."
https://drupal.org/node/1354#classes
actual fix: correct what this tests.
fixed.
getInfo() (and setUp()) have @inheritdoc now.
https://drupal.org/node/325974
#2007766: {@inheritdoc} in tests on setUp and getInfo
fixed.
nit.
functions third person verb to say what they do.
fixed.
a)
assertTrue doesn't seem to be testing what we want it to test.
-get() should return the setting. Let's check that, instead of checking if it evaluates to true.
b)
I also think we should test changing the setting, and seeing if it gets the new setting.
c)
And also deleting it and seeing if the setting is gone from the file.
a, b, c, not fixed yet.
nit, newline at end of class
"Leave an empty line between end of method and end of class definition:"
https://drupal.org/node/608152#indenting
fixed.
I'm not sure how we should name this. Or what docs for @params are needed.
https://drupal.org/node/1354#functions
didn't really help.
But this is the same as was in #1945226: Add language selector on menus.
ok.
---
summary:
Question: 2.
Concern: 8.
---
next: I'll try it manually.
Comment #13
marthinal CreditAttribution: marthinal commented@YesCT thanks for your help :)
2)
We want to clear using the bundle key (foo on the test attached).
8)
a) Checking the values.
b) Changing the settings.
c) Delete the bundle and verify that the settings were removed as expected.
Comment #14
ultimikeRemoved whitespace and added newline between function and end of class ("https://drupal.org/node/608152#indenting). I did this because YesCT harassed me.
Oh wait, I'm supposed to say, "YesCT made me to teach me how to make an interdiff".
Interdiff and patch attached.
-mike
Comment #15
YesCT CreditAttribution: YesCT commentedI'm reviewing this now.
Comment #16
cordoval CreditAttribution: cordoval commentedshouldn't this line be broken into a multiline array?
indentation mess up
missing trailing comma
if you are using the second argument why not removing the comment above?
missing trailing comma
same
Comment #17
YesCT CreditAttribution: YesCT commentedworking with @cordoval to find the standards links to reference those things, and make a new patch with an interdiff for those things. (since they are not core gates, we should just fix them as part of reviewing)
Comment #18
Gaelan CreditAttribution: Gaelan commentedI searched for other instances of this pattern, and all of the other ones are one line:
@YesCT said we should keep it, because the comment explicitly mentions the filename.
I think that all of the problems in #18 are fixed now.
Comment #19
Gaelan CreditAttribution: Gaelan commentedHere's a test-only version.
Comment #20
Gaelan CreditAttribution: Gaelan commentedLike the last test-only version, except this one actually only contains tests.
Comment #22
Gaelan CreditAttribution: Gaelan commentedComment #23
Gaelan CreditAttribution: Gaelan commentedAdded steps to reproduce.
Comment #24
Gaelan CreditAttribution: Gaelan commentedI think this is RTBC for nodes. If we also want to do comments, etc., within this same issue, then this Needs Work.
Comment #25
YesCT CreditAttribution: YesCT commentedtalked to alexpott, we will do comments and others here in this issue.
---
since we are going to do the other stuff here, we need to update the motivation section of the issue summary (and other parts that might be confusing).
I need some clarification on reproducing the empty machine names.
iirc, I think we need to do comments and custom blocks, but we need to check this.
Comment #26
kfritscheReworked the old patch.
Moved the tests to the alredy existing language config tests, so we don't need to create a complete new test class.
Also removed the strange entity_node_type_update function and did it, like we do in vocabularies, which is now also used for block types and comment types.
Added the necessary submit hooks every where, as I don't have any better solution yet and this solution works and is already used in core for that matter multiple times. I just unified it and used it everywhere...
Talked with Gabor, Andre and yched about the issue, because it feels wrong for me what we are doing here with the submit hooks. They agreed but nobody had another better solution yet, as a form element can't have a own submit handler. Which makes most of the time completley sense, but something like that we would need here.
We can't use any update/create hooks, as we don't have the form values there anymore and we don't know for which entity types we would need to do that. So a better generic solution is still needed.
Also updated issue summary.
Comment #28
Gábor HojtsyDoes this still happen at all now that #2355909: language.settings config is not scalable landed?
Comment #29
Gábor HojtsyDoes this still happen at all now that #2355909: language.settings config is not scalable landed?
Comment #30
Gábor HojtsyAlso even the tests don't apply anymore unfortunately.
Comment #31
penyaskitoChecked the tests added here, and I think we have the same coverage after #2355909: language.settings config is not scalable. Is there anything else we can save from here so contributors get recognized?
Comment #32
Gábor Hojtsy@panyaskito: you did #2355909: language.settings config is not scalable, so if you don't see what can be saved from here, than it is none. Thanks all for working on this one, and sorry it did not make it, and the refactoring landed. :/ It is at least a better solution for Drupal overall.