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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Postponed

Postponing 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.

Gábor Hojtsy’s picture

Status: Postponed » Needs review
FileSize
2.03 KB

Although #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.

dawehner’s picture

Interesting ... 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?

Gábor Hojtsy’s picture

We 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?

Gábor Hojtsy’s picture

Discussed 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.

Gábor Hojtsy’s picture

Now with the tests and the test only as well for comparison.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/profiles/standard/src/Tests/StandardTest.php
@@ -86,6 +88,18 @@ function testStandard() {
+    $names = $this->container->get('config.storage')->listAll();
+    $factory = $this->container->get('config.factory');
+    /** @var \Drupal\Core\Config\TypedConfigManagerInterface $typed_config */
+    $typed_config = $this->container->get('config.typed');

I thought we do use now \Drupal:: instead of $this->container-> but sure, this should not block this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2301045-complete-language-views-fixes-and-tests.patch, failed testing.

The last submitted patch, 5: 2301045-complete-language-views-fixes.patch, failed testing.

The last submitted patch, 6: 2301045-config-schema-test-only.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 6: 2301045-config-schema-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2301045-complete-language-views-fixes-and-tests.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.63 KB
2.03 KB

If 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.

Status: Needs review » Needs work

The last submitted patch, 15: 2301045-complete-language-views-fixes-and-tests-15.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
19.36 KB
748 bytes

@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 :)

vijaycs85’s picture

Looks good. Some review items/questions:

  1. +++ b/core/modules/node/config/install/views.view.content.yml
    @@ -111,13 +110,6 @@ display:
    -            translation_link:
    
    @@ -268,24 +260,6 @@ display:
    -        translation_link:
    
    +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -125,11 +124,6 @@ display:
    -            translation_link:
    
    @@ -511,58 +505,6 @@ display:
    -        translation_link:
    

    is it going to be a followup to get this field back to all these views?

  2. +++ b/core/modules/block/src/BlockBase.php
    @@ -344,6 +344,9 @@ public function blockForm($form, &$form_state) {
    +    // Remove the admin_label form item element value so it will not persist.
    +    unset($form_state['values']['admin_label']);
    +
    

    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'

Gábor Hojtsy’s picture

Issue summary: View changes

@vijaycs85: no, I updated the issue summary with reasons why we removed and did not add it back. Also explained in #5.

dawehner’s picture

+++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
@@ -174,22 +174,6 @@ protected function doTestAuthoringInfo() {
   /**
-   * Tests translate link on content admin page.
-   */
-  function testTranslateLinkContentAdminPage() {
-    $this->drupalLogin($this->administrator);

+++ b/core/modules/user/src/Tests/UserTranslationUITest.php
@@ -52,19 +52,4 @@ protected function getNewEntityValues($langcode) {
-   * Tests translate link on user admin list.
-   */
-  function testTranslateLinkUserAdminPage() {

These tests seems kinda helpful, is there a reason to remove that?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh BS

  • webchick committed 01d85e1 on 8.x
    Issue #2301045 by Gábor Hojtsy: Fixed Standard profile has views which...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

In 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!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks 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.

Gábor Hojtsy’s picture

Also the general issue for removing optional handling is in #2212081: Remove views optional handler handling.

xjm’s picture

Twig double-escaping issues should be filed as children of: #2297711: Fix HTML escaping due to Twig autoescape

vijaycs85’s picture

Status: Fixed » Closed (fixed)

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