Deprecate using numeric literals for 'version' in .info.yml files, require strings

Hi, in Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks.

This patch should fix the bug.

Bumped on this issue when updating to 11.3

The patch works, but for some reason I could not apply it through composer.

Attached is a remade version that does apply.

I just run into this issue.

Not sure, but maybe dev version of some modules could cause it too.
I got float(2) on the string(6) "Splide" module.
in the info file I have
# Add a fake version so local drush and Git checkouts do not fail dependencies.
version: 2.0

And maybe that breaks it?

It's not about dev, it's about using unquoted strings and then the default yaml parser interpretes anything with a single . as a float. Using '2.0' in the example from #5 would.

I did just run into this one as well, I think it makes sense to fix this and cast explicitly. Possibly the cast should be earlier, where we parse the info file. The fix must be a merge request though.

Also ran into this when upgrading to Drupal 11.3.3. Turns out my custom modules info.yml files had version: 1.0 instead of the proper quoted string version: '1.0'

Updating the version to a string allowed me to run the drush updb command successfully.

Parche #4 funciona correctamente

I rebased on 11.x but discussed with @alexpott, we both agree that this should be cast into a string in \Drupal\Core\Extension\InfoParserDynamic::parse, next to the existing version check. Then we can be certain that any version is always a string anywhere we use it.

Not doing that change myself because I think I can still RTBC it if someone does prepare that.

Probably goes without saying, but I also think InfoParserDynamic::parse() is the right place for this cast.

Drat. I'm adding some test coverage to this in core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php. Casting at this point doesn't fully solve it. I put:

version: 1.0

in a test case. It's parsed as 1, so when cast to a string, it's still just '1', not the '1.0' we're expecting. I'll push the test changes in a moment, but wanted to flag it here, first.

The test coverage showed that for consistency, we don't want a closing period on the new exception message. That's what https://git.drupalcode.org/project/drupal/-/merge_requests/14791/diffs?c... is about.

But here's the badness:

https://git.drupalcode.org/project/drupal/-/jobs/8688837/viewer#L7968

 ✘ Float like version
       ┐
       ├ Version that looks like a float should be a string
       ├ Failed asserting that two strings are equal.      
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊ -'1.0'
       ┊ +'1'
       │
       │ /builds/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php:212
       ┴

Per Slack thread about this, @alexpott suggested that the best solution to #17 is to fully deprecate using anything other than a string for the version. Created a CR about it, pushed a commit to add the @trigger_error() in this case and expand test coverage to verify the deprecation (and casting behavior), and opened a follow-up to convert this to an exception in D13: #3576313: Throw an exception in InfoParserDynamic::parse() for non-string versions.

Not sure if the deprecation message should be more accurate and say "... is deprecated in drupal:11.3.4 ..." or if we can leave it as "deprecated in drupal:11.3.0".

A few minor/nit-ish comments on the MR, but one about using expectUserDeprecationMessage() instead of expectDeprecation() per #3497124: Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage*() instead.

Thanks for the review, and the tip about #3497124: Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage*() instead. Applied all suggestions, and rebased to latest main.

Asked about the end of #18 in Slack, and this is only going to land for 11.4.x, not 11.3.x. Changed deprecation message and expectation.

MR comments addressed and tests pass. The CR looks good as well.

A couple things while reviewing the IS just now:

• The proposed resolution should be updated to match the MR
• Does there need to be a test for the specific issue in SystemRequirements? I think no, but not sure

I'm OK for RTBC once the IS has been updated if the test isn't deemed necessary.

Oops meant to go back to NW for IS update.

lgtm

Added a link to the CR to the release note snippet.

I updated credit.

I just have one question, is the deprecation just to handle the 1.0 -> 1 change from comment 15 and 16?

Or is there another case we're trying to cover?

Per the summary, if you have a float literal in the .info.yml file for a custom module, there are currently fatal errors in SystemRequirements from expecting a string and getting a float. The fix here is to cast any non-string scalar to a string.

The deprecation is to warn people that relying on this string cast is a bad idea (because of the 1.0 -> 1 change, and potentially other weirdness from YAML trying to guess/simplify your version string if it looks like a float or int). Instead of trying to do anything fancy to try to handle numeric literals during YAML parsing to preserve what you put in your .info.yml file, we're warning people they better wrap their custom version strings in ' marks to ensure the parser doesn't mess with them.

Per the CR, none of this matters for contrib modules hosted on d.o, since the d.o packaging script has always inserted the version (as determined by the tag being packaged) as a string. Huzzah ~20 years ago me. 😂

Just added my last question to the summary as a remaining task:

Decide if we want a 11.3.x / 10.6.x MR(s) with just the fix to cast floats to strings, without the new deprecation message.

I'd appreciate a MR for 11.3, one less patch to maintain, on the other side, if we're deprecating anyway, I need to change the affected modules anyway.

Re: fix-only backport. From @alexpott in Slack:

I would - seems risk free imo

https://git.drupalcode.org/project/drupal/-/merge_requests/14987 targets 11.3.x branch, but I checked locally and it will cleanly cherry-pick into 10.6.x.

Pipeline is fubar due to the #3576458: [regression] Subsystem and Topics maintainers require access to re-run, trigger, or view tests evilness. Pipeline is running in the parent (Drupal) project, and I don't have permissions there to trigger the downstream full test job. 😢 However, the core unit tests already passed, which is the only thing this should impact. Wouldn't hurt to have someone other than me review the MR diff, but it's basically identical to the orig without the @trigger_error() and expecting the deprecation in the test.

I did keep the new exception for non-scalar version, since this is definitely completely broken:

version:
  - WTF
  - Is
  - This

😂

@dww as a workaround you can create a second account.

You need to create the account, then go to that account on you main account to verify, then accept drupal terms of service, then accept terms of service on gitlab from gitlab code.

Then you can rerun jobs. Not ideal, but it's a workaround for the moment, I just tested it here.

Edit: I confirmed with @hestenet on slack that it's acceptable to do this.

I ran the 11.3.x pipeline manually and it passed finally.

YAML does use the term 'attribute' for the values.

I've updated the change record, so that should be checked.

Thanks for the review! Applied the suggestions. I was feeling icky calling them "attributes" (since that means something specific in PHP), but wasn't sure what else to use. 😅 Glad "value" is legit YAML-speak.

The CR edits mostly looked good, thanks for those, too. I did a few more tiny edits:
https://www.drupal.org/node/3576311/revisions/view/14296793/14296798

Back to NR. Hopefully the pipeline works, since it's running in the parent project and no one but core committers has permission to re-run individual jobs in there...

Thanks again,
-Derek

Re 34 assuming you meant does not use attribute, the Cr looks great!

However the main MR isn't passing tests so leaving it needs work

Edit crosspost, needs work though for the above.

Pretty sure https://git.drupalcode.org/project/drupal/-/jobs/8952387 are random package_manager build test fails. They all passed in the previously green pipeline: https://git.drupalcode.org/project/drupal/-/pipelines/758467

Since I'm a "developer", this pipeline ran in the parent, so only core-committers can re-run that job. Doesn't seem worth the $$ for a whole new pipeline to hope the random passes, so back to NR.

Indeed: #3508109: [random test failure] Package manager random build failures

Also, just double-checked the 11.3.x / 10.6.x fix-only MR. There are no deprecations to fix. The only user-facing text there is:

      elseif (!is_scalar($parsed_info['version'])) {
        throw new InfoParserException("The 'version' value must be a scalar in $filename");
      }

So that should be good to go as-is.

Thanks,
-Derek

To be extra clear, that exception is being added to give a more reasonable WSOD message if you do something truly weird in your .info.yml file.

Current 10.6.x local site where I broke a contrib module with this in its .info.yml:

version:
  - WTF
  - Is
  - This?

Right now, visiting extend page (or much of anything), results in:

The website encountered an unexpected error. Try again later.

TypeError: strlen(): Argument #1 ($string) must be of type string, array given in strlen() (line 395 of core/lib/Drupal/Component/Utility/Unicode.php).
Drupal\Component\Utility\Unicode::validateUtf8(Array) (Line: 65)
Drupal\Component\Utility\Xss::filter(Array, Array) (Line: 857)
Drupal\Core\Render\Renderer->ensureMarkupIsSafe(Array) (Line: 437)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 248)
Drupal\Core\Render\Renderer->render(Array) (Line: 195)
template_preprocess_system_modules_details(Array, 'system_modules_details', Array)
...

With the MR diff applied:

The website encountered an unexpected error. Try again later.

Drupal\Core\Extension\InfoParserException: The 'version' value must be a scalar in modules/contrib/date_filter/date_filter.info.yml in Drupal\Core\Extension\InfoParserDynamic->parse() (line 84 of core/lib/Drupal/Core/Extension/InfoParserDynamic.php).
Drupal\Core\Extension\InfoParser->parse('modules/contrib/date_filter/date_filter.info.yml') (Line: 551)
Drupal\Core\Extension\ExtensionList->createExtensionInfo(Object) (Line: 317)
Drupal\Core\Extension\ExtensionList->doList() (Line: 155)
Drupal\Core\Extension\ModuleExtensionList->doList() (Line: 283)
...

Meanwhile, on that same local 10.6.x site, if I change date_filter.info.yml to include this:

version: 1.1

and visit update.php, I get WSOD with:

The website encountered an unexpected error. Try again later.
TypeError: array_pop(): Argument #1 ($array) must be of type array, false given in array_pop() (line 432 of core/modules/system/src/Controller/DbUpdateController.php).

But if I apply the backport MR diff, I get a working DB update process. 🎉

Needs manual testing 😅

Those updates look good, tests are passing again!

Thanks @nicxvan!

Minor formatting fixes to remaining tasks.

Committed and pushed 1bbd5cf0cb7 to main and 1c1e7f6d59f to 11.x. Thanks!
Committed and pushed d7a1176d78a to 11.3.x and 41b66710ac6 to 10.6.x. Thanks!