Problem/Motivation
Several configuration elements have tests for schema compliance but not all. Using capabilities in latest 8.x and the config_inspector module, people can review compliance of the configuration at any point in time on their site with the schema. While there may be (and likely are) holes in the schema for a well developed site, even a default installation of Drupal 8 has schema holes. When installing the standard profile only, there are issues fixed in #2295737: Not all shipped configuration passes validation even with all modules enabled and additionally optional elements used in views that cause problems.
- views.view.content
- display.default.display_options.fields.translation_link.text and display.default.display_options.filters.langcode.value missing schema.
- views.view.user_admin_people
- display.default.display_options.fields.translation_link.text missing schema.
Once providing test for the fixes, another issue with admin_label for blocks ending up in configuration was identified.
Proposed resolution
In discussion of the views ones with @alexpott and as per him, discussion in Austin resulted in that optional dependencies like that in config entities will not be possible. The only way to get those translation links would be that content translation would need to provide an alternate view on its own for the node and user admin pages. However the feature may not be significant enough to warrant a copy of the view and confusion for the user to select. So we opted not to provide that here and loose this feature instead. Users can add this to the view easily.
For the block admin_label problem, an 'item' type field was identified in BlockBase. The item has no value (only markup), so ends up as an empty string in the data, which makes it appear in the saved configuration. Removing the element from the form data in validation lets us keep the item's visual/structural form without affecting the configuration.
Remaining tasks
Review.
User interface changes
The node admin and user admin views will not have a built-in translation operation in the operation links, even if content translation is enabled. Admins can add this back as part of possible other customizations to these views on the site.
API changes
Language views plugins are all moved to views. No API changes other than the placement of the plugins changes, so anything depending on these plugins (unlikely) would need to change their use statements.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff.txt | 748 bytes | Gábor Hojtsy |
#17 | 2301045-complete-language-views-fixes-and-tests-17.patch | 19.36 KB | Gábor Hojtsy |
#15 | interdiff.txt | 2.03 KB | Gábor Hojtsy |
#15 | 2301045-complete-language-views-fixes-and-tests-15.patch | 18.63 KB | Gábor Hojtsy |
#6 | 2301045-complete-language-views-fixes-and-tests.patch | 16.6 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyPostponing on #2295737: Not all shipped configuration passes validation even with all modules enabled since we cannot add tests for this until that is fixed. Several fails would overlap.
Comment #2
Gábor HojtsyAlthough #2295737: Not all shipped configuration passes validation even with all modules enabled did not land yet, let's try moving forward here. So long as we don't try to add tests, the changes should pass hopefully.
Comment #3
dawehnerInteresting ... views_language_list() does indeed just depend on the language manager so we are safe here.
Should we also mark the language filters as not optional?
Comment #4
Gábor HojtsyWe should also move the config schemas of course, which made me realize the other two language provided views plugins can also be views native. All they depend on are views_language_list and language_load both of which are language manager provided, not in language module.
@dawehner: not sure where to mark them not optional?
Comment #5
Gábor HojtsyDiscussed with @dawehner, he meant the optional views elements in the content/people admin views. Yeah those were the remaining changes. We still need tests, but the test will uncover more fails due to #2295737: Not all shipped configuration passes validation even with all modules enabled not yet committed (which fixes those). I think I'll cook up a test anyway.
Also, I looked into copying the view altogether in content translation but not sure it is worth it for a dropbutton operation to reach translations direct to do that and then endure the pain of double/parallel maintenance for the two copies.
Comment #6
Gábor HojtsyNow with the tests and the test only as well for comparison.
Comment #7
dawehnerI thought we do use now \Drupal:: instead of $this->container-> but sure, this should not block this.
Comment #15
Gábor HojtsyIf we don't have that translation feature, the tests don't make sense either. The remaining fail about the admin_label is not very clear to me. I can reproduce locally that the admin_label somehow gets into this block. I can reproduce this placing a menu block and resaving a shipped menu block. Not resaving other blocks, eg. a search block. So somehow related to the menu derivatives but not clear how that will end up in saved config. And it is always empty.
Comment #17
Gábor Hojtsy@vijaycs85 helped debug and figured out the reason. I mis-stated that it did not happen for other blocks, it does happen for other blocks, even for the search block. The reason is BlockBase has a form 'item' type element to display the admin label. It does not have a value, but it does have markup. So the value will always be empty. Form 'item' types end up in form values, so there is your empty admin_label persisting in config for no reason at all. It is never used from there and has no business in config (it is not configurable). So we should not persist the value. A straightforward solution is to unset the form value before it reaches any processing.
The test-only patch shows all the fails with admin_label and the views in #6.
I think this is now done. This one should be green :)
Comment #18
vijaycs85Looks good. Some review items/questions:
is it going to be a followup to get this field back to all these views?
Opened #2304683: Non-config elements getting into config file increase if we have anything out there other than block's 'admin_label' and shortcut's 'links'
Comment #19
Gábor Hojtsy@vijaycs85: no, I updated the issue summary with reasons why we removed and did not add it back. Also explained in #5.
Comment #20
dawehnerThese tests seems kinda helpful, is there a reason to remove that?
Comment #21
dawehnerOh BS
Comment #23
webchickIn testing this I found a couple of regressions. One is that the translation overview page has a "<span ...>" in it, a follow-up from the Twig auto escape patch. The second is that the language switcher block doesn't actually filter the node frontage view. This is an existing issue in HEAD and not touched by this patch. The third is that the admin content view's filters also seem to be broken: creating a french + english node and filtering the view by "French" shows me 0 nodes instead of the translated 1 that it should show me. So we need follow-ups for all of those, but none of them are influenced by this patch, which basically is just removing code and moving it around.
Tim also noted that the last hunk changing Block labels is weird, and would like a follow-up to discuss that further.
Given this patch itself is pretty simple, committed and pushed to 8.x. Thanks!
Comment #24
Gábor HojtsyThanks a lot!
1. The node admin filter (and pre-requisite for the front page fix) is at #2217943: Views cannot be filtered by language of translation.
2. The front page issue which then can be resolved is at [#8576551].
3. Is there a process / tag / place for opening the TWIG autoescape issues?
4. Opened #2305743: Make block admin code more config compatible so admin label will not need workaround for the block item.
Comment #25
Gábor HojtsyAlso the general issue for removing optional handling is in #2212081: Remove views optional handler handling.
Comment #26
xjmTwig double-escaping issues should be filed as children of: #2297711: Fix HTML escaping due to Twig autoescape
Comment #27
vijaycs85@jhodgdon raised #2305799: HTML tags (raw) shown in Status column on Translations page under #2297711: Fix HTML escaping due to Twig autoescape
Just to summaries the followups: