Problem/Motivation

In #2313917-108: Core version key in module's .info.yml doesn't respect core semantic versioning we are changing the core: key in .info.yml file to support more than 8.x, for example ^8 || ^9 or ^8.6.7

Because there is core key server urls used by the Locale module we were worried that changing this key would affect the urls.

It appeared in locale_translation_build_projects() t
'core' => isset($data['info']['core']) ? $data['info']['core'] : \Drupal::CORE_COMPATIBILITY,
Was setting the core key from info.yml files if set.

But because \Drupal\Core\Utility\ProjectInfo::filterProjectInfo() filters out the core key the value will never originate from the .info.yml file

The problem is that this not clear that this was intentional and that there is test coverage that would break if it were to change, for example by adding 'core' to the hardcoded whitelist in \Drupal\Core\Utility\ProjectInfo::filterProjectInfo() or to the $additional_whitelist whitelist sent from locale_translation_project_list()

Proposed resolution

Add a test that proves that the altering the core and name keys will not affect these keys as returned by

Remaining tasks

Make the test

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#5 3070354-5.patch3.5 KBWim Leers
#5 interdiff.txt1.17 KBWim Leers
#4 3070354-4.patch3.51 KBWim Leers
#4 interdiff.txt3.75 KBWim Leers
#2 3070354-2-new-test.patch3.53 KBtedbow
#2 3070354-2-locale-no-filter-core-FAIL.patch6.08 KBtedbow
#2 3070354-2-locale-no-filter-core.patch2.55 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
3070354-2-locale-no-filter-core.patch2.55 KB
3070354-2-locale-no-filter-core-FAIL.patch6.08 KB
3070354-2-new-test.patch3.53 KB

Here are a few of patches

  1. 3070354-2-locale-no-filter-core.patch adds 'core' to the whitelist. This should pass proving there is no test coverage currently
  2. 3070354-2-locale-no-filter-core-FAIL.patch has above plus the new test.
  3. 3070354-2-new-test.patch has just the new test(this would be committed)

1,2 have drupalci.yml changes to only run locale module tests.

The last submitted patch, 2: 3070354-2-locale-no-filter-core-FAIL.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Add a test for locale_translation_build_projects() to unensure that 'core' does not affect translations URLs » Add a test for locale_translation_build_projects() to unensure that 'core' key does not affect translations URLs
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MI, +Drupal 9
Related issues: +#2313917: Core version key in module's .info.yml doesn't respect core semantic versioning
FileSize
interdiff.txt3.75 KB
3070354-4.patch3.51 KB

#2313917-110: Core version key in module's .info.yml doesn't respect core semantic versioning.2 pointed me here. I've read the related comments there. Excellent find! This patch removes some ambiguity, it enables us to make changes with confidence. Well done! 👍

Found a bunch of nits. Fixed them all.

  1. +++ b/core/modules/locale/tests/modules/locale_test/locale_test.module
@@ -26,6 +26,15 @@ function locale_test_system_info_alter(&$info, Extension $file, $type) {
+  // Alter the name and the core version of the project. This should not effect

    Nit: s/effect/affect/ 

  2. +++ b/core/modules/locale/tests/src/Kernel/LocaleBuildTest.php
@@ -0,0 +1,64 @@
+ * Tests for building the translatable project information.

    s/Tests for/Tests/ 

  3. +++ b/core/modules/locale/tests/src/Kernel/LocaleBuildTest.php
@@ -0,0 +1,64 @@
+   * Checks if a list of translatable projects gets built .

    Nit: s/built ./built./ 

  4. +++ b/core/modules/locale/tests/src/Kernel/LocaleBuildTest.php
@@ -0,0 +1,64 @@
+    $this->assertEquals('locale_test', $projects['locale_test']->name);

    Nit: we should use assertSame() wherever possible. 

  5. +++ b/core/modules/locale/tests/src/Kernel/LocaleBuildTest.php
@@ -0,0 +1,64 @@
+    // Confirm the name and core value are changed in  $module->info.

    Nit: s/in $module->info/in $module->info/

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff.txt1.17 KB
3070354-5.patch3.5 KB

I forgot to fix points 3 and 5. Done now.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

Shouldn't we be doing something to the (presumably) dead code path in locale.compare.inc here? Removing it, or at least an @todo or deprecation or something?

Also what about the seemingly related use of the constant in install_check_translations()?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Related note from Gábor: #2313917-107: Core version key in module's .info.yml doesn't respect core semantic versioning

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +mwds2019
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

re #7

  1. Add follow up #3073934: Remove dead code path in locale_translation_build_projects that was meant to set 'core' for localization path from project 'core'
  2. install_check_translations() uses '%core' => \Drupal::CORE_COMPATIBILITY, but it doesn't have any code that appears to the get the 'core' key from project info

re #7
I think @Gábor was referring to this issue #3016471: Make localization file download major version agnostic it is postponed on some d.o changes

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

While #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning does not actually want to change the core key anymore, it will be possible that a core key will not be present after that patch. Since that patch is not yet in, this does not need to test for a missing core key, but that would be the next step IMHO add test coverage for at least for this case. I think this looks fine to cover testing what this meant to.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Fixed

  • Gábor Hojtsy committed 520bbcd on 8.8.x 
    Issue #3070354 by tedbow, Wim Leers, xjm, Gábor Hojtsy: Add a test for...

Status: Fixed » Closed (fixed)

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