Problem/Motivation

Follow up to #3118087: If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status

We needed remove version: VERSION from aaa_update_test.info.yml because want to test modules with no version number.

Other *_update_test.info.yml files have version: VERSION. We should remove those as we those aren't used for testing and update module is special case because we mocking version numbers.

Proposed resolution

Remove version: VERSION from *_update_test.info.yml files files

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3120961-5.8_x.patch1.48 KBdww
#5 3120961-5.9_x.patch1.44 KBdww

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Related issues: +#3118087: If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: Remove VERSION from update test module which used dynamic version numbers » Remove VERSION from update test module info.yml file which use dynamic version numbers in tests
dww’s picture

dww
we/he/they
commented

Big +1 to this. However, I don't understand point 2 in the proposed resolution:

Determine if should still have at least module with version: VERSION to test contrib having version numbers.

Huh? All the tests involve testing contrib having version numbers. ;) What does this mean?

Thanks,
-Derek

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 3120961-5.9_x.patch1.44 KB
new 3120961-5.8_x.patch1.48 KB

3120961-5.8_x.patch applies cleanly to both 8.9.x and 8.8.x branches.

Tests are still running locally, but so far so good. Let's see what the d.o bot thinks...

naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Assigned: Unassigned » naresh_bavaskar
naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Assigned: naresh_bavaskar » Unassigned
Status: Needs review » Reviewed & tested by the community

above patch LGTM

The last submitted patch, 5: 3120961-5.9_x.patch, failed testing. View results

neslee canil pinto’s picture

neslee canil pinto
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

@naresh_bavaskar, can you please do a dry run test.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

@Neslee Canil Pinto re: #9: There's nothing to screenshot here. It's an internal change to test-only modules. The testbot shows that all the tests still pass with this change applied. I don't think there's anything to gain from a "dry run test" nor attaching screenshots of anything.

There was random failure weirdness on the 9.0.x branch -- I requeued that since the 75 failures along these lines obviously have nothing to do with this patch:

1) Drupal\Tests\block_content\Functional\BlockContentTranslationUITest::testTranslationUI
Translation date correctly stored.
Failed asserting that '1586016635' matches expected 1586013035.

Back to RTBC.

Thanks,
-Derek

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Remove VERSION from update test module info.yml file which use dynamic version numbers in tests » Remove VERSION from update test modules that receive dynamic version numbers in tests
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Sorry yeah I am not sure I remember what I was thinking in 2) in the issue summary.

Plus 1 to RTBC

I couple of thoughts but both are existing issue and don't affect this issue

  1. We don't have any tests that actually use version that is set in the info.yml file. We always rely on the alter so we don't really have test coverage for it being picked up from the info.yml file. But that is existing issue and is not affected by this issue
  2. \Drupal\Core\Extension\InfoParserDynamic::parse() we have
    if (isset($parsed_info['version']) && $parsed_info['version'] === 'VERSION') {
        $parsed_info['version'] = \Drupal::VERSION;
      }

    This seems weird that we don't check if this is core file. Is there any way a contrib module could have
    version: 'VERSION'
    No, right? because drupal.org packaging sets this? Always? If they could then you would start to get weird situations where the update module would think the module version is \Drupal::VERSION. Would that cause any problems now that contrib can have semantic versioning?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs followup

I guess #12.2 is followup material?

xjm’s picture

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

NR for that followup to be filed.

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Needs review » Needs work

*sigh* #12.2 has nothing to do with this issue. It's not improved or harmed. We've never had that test coverage, and that concern is entirely about the .info.yml parsing system, not the update.module. But whatever. I'll open the follow-up.

dww’s picture

dww
we/he/they
commented
Assigned: dww » Unassigned
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup

#3132978: Add test coverage of version: VERSION handling in info parser system

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Removing point 2 from the proposed resolution, per #12.

  • xjm committed 309ec4c on 9.1.x 
    Issue #3120961 by dww, tedbow: Remove VERSION from update test modules...

  • xjm committed f71bdfc on 9.0.x 
    Issue #3120961 by dww, tedbow: Remove VERSION from update test modules...

  • xjm committed 05440ba on 8.9.x 
    Issue #3120961 by dww, tedbow: Remove VERSION from update test modules...

  • xjm committed b87c1f2 on 8.8.x 
    Issue #3120961 by dww, tedbow: Remove VERSION from update test modules...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.0.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 9.1.x, 9.0.x, 8.9.x, and 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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