Problem/Motivation

All views created in Drupal 8 have a root config key of core with the value set to \Drupal::CORE_COMPATIBILITY ie. 8.x

Proposed resolution

Discover why this information is needed in View (my suspicion is that this is a leftover from when Views was not in core and this information was move useful) - DONE: Not needed by core.

Remove this key using a update hook. - Done: @see views_post_update_remove_core_key()

Remaining tasks

  1. Write change record
  2. Final reviews/RTBC
  3. Commit

User interface changes

None

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

Mixologic’s picture

When I was auditing CORE_COMPATIBILITY I found this, but could not, anywhere, see that it was necessary or being checked.

Berdir’s picture

> Discover why this information is needed in View (my suspicion is that this is a leftover from when Views was not in core and this information was move useful)

Yes, IIRC this was carried over from views 7.x where it was used to detect version 2 or 3.

alexpott’s picture

Title: Decide what to do with the core key in views configuration. » Remove the core key from views configuration.
Version: 9.0.x-dev » 8.8.x-dev
Status: Active » Needs review
FileSize
112.05 KB
Wim Leers’s picture

  1. Updated the REST test coverage expectations.
  2. Many test views didn't specify core: 8.x but core: '8'. Another reason for #2869792: [meta] Add constraints to all config entity types
  3. Updated ViewStorageTest's expectations.
Wim Leers’s picture

Issue tags: +VDC, +Needs update path, +Drupal 9

This will also need an update path to convert all contrib/custom exported views.

alexpott’s picture

@Wim Leers it doesn't

need an update path to convert all contrib/custom exported views.

Because we're removing the core key from the exported property list in the annotation during module install it'll remove the key :)

Yes people need to re-export their views (or remove the key from exported config) but there's nothing more we can do in core.

Wim Leers’s picture

Issue tags: -Needs update path

Right, they don't need to change code. But they need to re-export.

We had to do something similar shortly before the Drupal 8 release. I recall that being considered too disruptive. We had to take extra measures to make that painless.

I should've phrased #6 more clearly: will my existing site continue to work, because the core key will just be ignored?

AHHHH I had missed views_post_update_remove_core_key() 🤦‍♂️🤦‍♂️🤦‍♂️🤦‍♂️ Sorry for the distraction 😅

dww’s picture

Issue summary: View changes
Issue tags: +Needs change record

+1 to this. The core: key in the views config is cruft at this point. We don't have a system any more for version-dependent views config schemas or anything like that. If/when we decide we need it, we'll need to add real support for it. At this point, nothing cares.

However, this seems worthy of a CR, especially if contrib maintainers are expected to remove this key from default views they're shipping, etc.

Thanks,
-Derek

Status: Needs review » Needs work

The last submitted patch, 5: 3087692-5.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
200.21 KB
  1. JSON:API test coverage expectations updated.
  2. Apparently a bunch of test views specify core: 8.0-dev 🤦‍♂️🤦‍♂️🤦‍♂️ — see #5.2 again.
rensingh99’s picture

Assigned: Unassigned » rensingh99
Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs change record

What's left here is only review + RTBC. @alexpott already wrote a change record (https://www.drupal.org/node/3087832) minutes after #9 was posted :)

Krzysztof Domański’s picture

Status: Needs review » Needs work

I found additional views configuration files with containing core key.

find core -name '*view*.yml' -exec grep -r 'core:.*8' {} \; -print

(...) // *.info.yml files
14:core: 8.x
core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2455125.yml
14:core: 8.x
core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2846614.yml
14:core: 8.x
core/modules/system/tests/fixtures/update/drupal-8.views-taxonomy-parent-2543726.yml
16:core: 8.x
core/modules/system/tests/fixtures/update/drupal8.views-image-style-dependency-2649914.yml
(...) // *.info.yml files
dww’s picture

Status: Needs work » Needs review

@Krzysztof Domański - those are fixtures for automated tests to validate DB schema updates. The entire point of those files is to have "stale" data. We could never test this change if we "fix" the fixtures. Those files need to represent the "old" data in Drupal, so tests can start from known old states, run their updates, and assert that the old stuff got correctly converted to the new stuff.

Back to NR.

Wim Leers’s picture

Version: 8.8.x-dev » 8.9.x-dev

Still applies cleanly. Retesting.

rensingh99’s picture

Assigned: rensingh99 » Unassigned
Status: Needs review » Reviewed & tested by the community

Hi,

I have reviewed the patch # 11 and it worked as designed. It’s removed the core key from all views which is included in core.
Now core key is also removed if we export the configuration for any view.

Export screenshot before applying patch.
Only local images are allowed.

Export screenshot after applying patch.
Only local images are allowed.

Thanks,
Ren

  • catch committed bc6aabc on 9.0.x
    Issue #3087692 by Wim Leers, alexpott: Remove the core key from views...

  • catch committed 6b44ddc on 8.9.x
    Issue #3087692 by Wim Leers, alexpott: Remove the core key from views...

  • catch committed 1c8f042 on 8.8.x
    Issue #3087692 by Wim Leers, alexpott: Remove the core key from views...
catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.0.x, cherry-picked to 8.9.x and 8.8.x, thanks!

Wim Leers’s picture

Published change record: https://www.drupal.org/node/3087832.

rensingh99’s picture

Hi,

I have reviewed this patch but I did not get any credit.
Please advise me on how I can improve for the next issues.

Thanks
Ren

catch’s picture

I've retrospectively added commit credit for this. Was a bit borderline for me because we have explicit test coverage in the patch, so the issue did not really need manual testing, but the manual testing that you did was fine.

Status: Fixed » Closed (fixed)

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