Views content language field default configuration should use field_language plugin

Created on 6 July 2024, 3 months ago
Updated 13 September 2024, about 1 month ago

Problem/Motivation

views.view.content.yml uses 'field' as the plugin for the language field, yet 'field_language' plugin exists for this purpose
(and is used because of overrides in views_data anyways). The config should match the result rather than relying on an
override here (ie, the config should match that field_language plugin is used.

The schema should be updated to match.

This was revealed as a symptom of the problem being addressed in πŸ› Views handler loading should respect configuration Active and was suggested by @catch to tackle in a separate issue.

Steps to reproduce

This was revealed as a symptom of the problem being addressed in πŸ› Views handler loading should respect configuration Active , if the views handler manager respects the configuration, then this view configuration would have issues.

Proposed resolution

The views.view.content.yml optional install should be updated to match what would happen if you configure it via the UI, it should not be outdated configuration. Resave the View and copy the applied change to the plugin_id to set the Translation Language to 'field_language' instead of 'field'. Ensure that configuration validation for it passes since the field_language plugin is currently missing schema.

Remaining tasks

Merge request review

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

The default view configuration for the Admin > Content view now uses the correct Language field plugin.

Draft change record: https://www.drupal.org/node/3460053 β†’

πŸ› Bug report
Status

Fixed

Version

10.4 ✨

Component
ViewsΒ  β†’

Last updated about 1 hour ago

Created by

πŸ‡¬πŸ‡§United Kingdom scott_euser

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @scott_euser
  • Status changed to Needs review 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Not sure if there is a way for the schema of views.field.field_language to just inherit everything from views.field.field? It would make sense to inherit if we can since the 'Language' field plugin extends 'Field' field plugin with nothing other than an access control method (so that the field does not show up in a monolingual site).

  • Pipeline finished with Success
    3 months ago
    Total: 558s
    #217170
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    I wonder if its also worth having an update hook to update the views config of existing sites if its unchanged?

  • πŸ‡¬πŸ‡§United Kingdom catch

    I wonder if its also worth having an update hook to update the views config of existing sites if its unchanged?

    We didn't do that with the original issue (which means a relatively small number of sites will be affected by the fact the shipped config wasn't updated in the original issue) because there's always risk of breaking customisations if the detection goes wrong etc. I think it's fine without. Should probably add a new change record encouraging people to update their admin/content views to get the new behaviour though.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Sounds good, here is a draft change record: https://www.drupal.org/node/3460053 β†’

  • Status changed to RTBC 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems straight forward.

    Applied the MR before doing an install and filter appears to be working.

    Imagine this is one of those views it would be more disruptive to post_update existing sites then to just fix for new ones.

  • Status changed to Needs work 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    I think we still need feedback on #3 here, adding all that config schema seems like a lot.

  • Status changed to Needs review 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Okay @alexpott helped solve that extending here https://drupal.slack.com/archives/C1BMUQ9U6/p1720626481941179 (thank you!).

    I tested via πŸ› Views handler loading should respect configuration Active running core/modules/views/tests/src/Functional/Entity/BaseFieldAccessTest.php

    • Without any `views.field.field_language` schema: test fails
    • With it in place and `type: views.field.field` extending: test passes
  • Pipeline finished with Success
    3 months ago
    Total: 457s
    #221097
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Okay added back in field_api_classes - confirmed that the test still passes in πŸ› Views handler loading should respect configuration Active with this.

  • Pipeline finished with Success
    3 months ago
    Total: 445s
    #221103
  • Status changed to RTBC 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Looks good to me now!

  • Status changed to Needs work 3 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    I read the IS, comments and MR. There are no unanswered questions here. The proposed resolution should explain what the agreed solution is.

    I read the change record and I think it needs updating. I think it should stated that all views using the 'Translation language' field need to be updated and that there is an addition to the views field schema. It is not clear to me why the re-save is not needed on multi-lingual sites using the field in a view. Setting to NW for updating the change record. Oh, and it needs the branch/version added as well.

    I do want to remind everyone that discussions in Slack should be summarized in the issue. That is the only way to ensure we have a record of the conversation.

  • Status changed to Needs review 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    I read the change record and I think it needs updating. I think it should stated that all views using the 'Translation language' field need to be updated

    Hmmm I am not sure if that is the case. If you create a new view with the Translation Language field you get the correct field_language plugin (just re-checked for sanity). The problem is that the config install in core/modules/node/config/optional/views.view.content.yml was outdated. If I understand the situation right, it stems from the underlying problem that there is no validation or update hooks covering anything in config/install/* or config/optional/* anything there (including in Core) slowly get out of date. This is just a symptom of that wider problem, but will become an issue if πŸ› Views handler loading should respect configuration Active gets in. So I do think it is really isolated to this specific view.

    So if I understand it right (maybe @catch you can confirm), once πŸ› Views handler loading should respect configuration Active lands it is only monolingual sites that have a problem and only on the /admin/content View as the field plugin would then show the Translation Language column whereas it currently automatically hides that because Views does not respect the configuration of the View.

    So we are currently letting people install a problem, and the goal here is to at least stop new sites from installing that same problem: the outdated views.view.content.yml.

    I do want to remind everyone that discussions in Slack should be summarized in the issue. That is the only way to ensure we have a record of the conversation.

    Apologies for that. Actually this came from MR comments https://git.drupalcode.org/project/drupal/-/merge_requests/8588#note_336534 where @catch suggested:

    This might be worth splitting out to its own issue since it seems like a straight bugfix of the previous change to add this filter. Could backport to 10.4 then potentially.

    The Slack conversation was not about the proposed resolution, just because I did not know how to answer my own question from #3 on whether there was a better way to code this sorry

    Not sure if there is a way for the schema of views.field.field_language to just inherit everything from views.field.field?

    And @alexpott answered

    type: other_field

    to point me in the right direction. Still learning config schema stuff, all a bit of a black box to me still.

    Setting to NW for updating the change record. Oh, and it needs the branch/version added as well.

    Can someone help me with that please? I do not know the policy there; is that 11.x for where it ideally originally gets committed, or would that be 10.4.x where the proposed back-port is from @catch's comment? For now I just make a guess at 11.x / 11.1.0 looking at other change records, in order to be able to move this issue back to Needs Review, but I have zero confidence in this.

    For now I updated the second paragraph text to be this:

    Existing monolingual sites should resave the 'Translation language' field in the Admin > Content View (found at /admin/structure/views/view/content when Views UI is enabled) so that in the future the 'Translation language' does not start appearing when only one language is available on the site.

    Should I be referencing an unsolved issue πŸ› Views handler loading should respect configuration Active in there rather than just saying 'in the future'? Sorry for my ignorance here, change record writing also new to me obviously - still learning Core contributions.

    Also added a third paragraph for clarity:

    Multilingual sites can re-save this as well, but it is not necessary since the 'Language' plugin's only change is to hide itself on monolingual sites.

    The proposed resolution should explain what the agreed solution is.

    Sorry about that! I updated the issue summary to be me clear on the code change. Would welcome any further feedback there if I should add more details.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thanks for the details. Is there any blocker for this?

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    I don't think so, I believe everything from #12 is addressed.

  • Status changed to RTBC about 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Going to go on a limb and say so too. Thanks!

    • catch β†’ committed 88ebf6f2 on 10.4.x
      Issue #3459496 by scott_euser, catch, smustgrave, quietone: Views...
    • catch β†’ committed fdc8b43e on 11.0.x
      Issue #3459496 by scott_euser, catch, smustgrave, quietone: Views...
    • catch β†’ committed fe48edb1 on 11.x
      Issue #3459496 by scott_euser, catch, smustgrave, quietone: Views...
  • πŸ‡¬πŸ‡§United Kingdom catch

    Yep this all looks good to me.

    Committed/pushed to 11.x and cherry-picked to 11.0.x, 10.4.x and 10.3.x, thanks!

  • Status changed to Fixed about 2 months ago
    • catch β†’ committed aa8e3573 on 10.3.x
      Issue #3459496 by scott_euser, catch, smustgrave, quietone: Views...
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024