Problem/Motivation
API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
The documentation does not mention that there is a key called core_version_requirement and that it is a required field.
Steps to reproduce
Proposed resolution
Document the core_version_requirements key in \Drupal\Core\Extension\InfoParserInterface in the 'Information stored in all .info.yml files:' section.
Remaining tasks
Make a patch
Review
Commit
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff_3080819_24-26.txt | 1.31 KB | ankithashetty |
#26 | 3080819-26.patch | 960 bytes | ankithashetty |
#24 | interdiff_19_24.txt | 924 bytes | beatrizrodrigues |
#24 | 3080819-24.patch | 1.08 KB | beatrizrodrigues |
#19 | interdiff_17_19.txt | 792 bytes | beatrizrodrigues |
Issue fork drupal-3080819
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Vivek Panicker CreditAttribution: Vivek Panicker commentedComment #4
quietone CreditAttribution: quietone at PreviousNext commentedThe key 'core' has been deprecated, info.yml files no longer accept 'core: 9.x' and replaced with 'core_version_requirement', New 'core_version_requirement' key in info.yml files for modules, themes and profiles allows Composer semantic version constraints including specifying multiple major versions of core. However, 'core_version_requirement' is not documented on \Drupal\Core\Extension\InfoParserInterface like the other keys, such as 'package'. Therefore updating the IS to add documentation for 'core_version_requirement'. I reckon this is suitable as a Novice issue.
Since this is a documentation only change, when uploading the patch, select 'Do not test' (and save on resources). The tests can be run when this is ready for RTBC.
Edit: fix links
Comment #5
quietone CreditAttribution: quietone at PreviousNext commentedForgot to update the title.
Comment #6
beatrizrodriguesComment #7
beatrizrodriguesHere's the patch with the documentation for "core_version_requirement". I hope the description fits.
Comment #8
anagomes CreditAttribution: anagomes at CI&T commentedThe patch in #7 applies correctly. It briefly describes the core_version_requirement key and makes it clear what it means and that it is required in the info.yml file. Looks good to me!
Comment #9
quietone CreditAttribution: quietone at PreviousNext commentedI have fixed the links in my earlier comment.
@beatrizrodrigues, thanks for making a patch and not running the tests yet.
@anagomes, thanks for your interest. Before a patch can be RTBC it also needs to pass tests. Yes, even for a simple documentation change. There is more detail about the steps in core gate
This should also that core_version_requirement applies to themes and profiles as well. Of course, it didn't help that the links in my comment were broken. They are fixed now.
I also think we should go one step further and show how to add multiple versions. Something like this? Change as you see fit.
core_version_requirement: Specifies the version or versions of Drupal core. When the project is compatible with more that one version separate the versions with "||". For example, for a project compatible with both Drupal 9 and 10 use ^9 || ^10. (Required)
The use of (Required) at the end is not needed, instead optional parameters are identified, as per Making lists in documentation. However, because Required is used elsewhere in this file I think that should stay. If anything, a followup can be made to improve the Interface document by cleaning up the Required and adding complete examples. Although, there may be an issue for that already and I am not able to search right now.
Comment #10
beatrizrodriguesthank you for your feedback @quietone
The description gets really better like this, indeed. Thank you for the links and for sharing your knowledge with me. I'll apply the improvements to the documentation and send the patch here. And I will run the tests for it. Thank you again!
Comment #11
beatrizrodriguesI think the description you gave @quietone fits perfectly, so I applied it. Here is the patch where I did the improvement suggested, and a interdiff too. (I searched for the issue about cleaning up the Required but I was not able to find it, i'll keep searching, because I would gladly work on that.) Thanks again.
Comment #13
lucienchalom CreditAttribution: lucienchalom at CI&T commentedThe last patch apply cleanly and I liked and agree with the addition of the example.
moving to RTBC.
Comment #14
quietone CreditAttribution: quietone at PreviousNext commented@beatrizrodrigues, thanks again.
Oh no, I see a typo.
s/../.
Comment #15
beatrizrodriguesThanks @quietone I hope all the typos were fixed. Here is a patch and a interdiff.
edit: Sorry, I forgot to make the interdiff with .txt extension.
Comment #16
quietone CreditAttribution: quietone at PreviousNext commentedI like the additional comma, it reads better, but there is still an instance of two periods.
This should be '.' not '..'.
Comment #17
beatrizrodriguesThanks @quietone, I was missing that extra ".". Here's the new patch and interdiff.
Comment #18
quietone CreditAttribution: quietone at PreviousNext commentedI don't believe I missed another one! 'uses' should be 'use'. Looking back, that change happened in #15 and obviously I was looking at the '..'.
I applied the patch and read the change (slowly) and realized that it would be better with quotes. Since this needs a change for the word 'use' we might as well do this one too. Here is what I suggest:
* with both Drupal 9 and 10 use "^9 || ^10", without quotes. (Required)
Comment #19
beatrizrodriguesthanks again @quietone Here is the patch and the interdiff with the changes suggested.
Comment #20
beatrizrodriguesComment #21
paulocsI'm not a native English speaker but it looks good to me. It's very clear for me and I don't see any typo.
Moving to RTBC.
Comment #22
quietone CreditAttribution: quietone at PreviousNext commented@beatrizrodrigues, thanks for seeing this through to RTBC.
Comment #23
catchI think this looks good, but... could we also mention the possibility of using >= here? For example
>= 8
to allow compatibility with Drupal 8, 9 and 10 (and later versions) without needing to update each major release.Comment #24
beatrizrodrigues@catch thanks, I adapt a little bit you suggest. Please, let me know if it fits.
Comment #25
quietone CreditAttribution: quietone at PreviousNext commentedThat is interesting. What catch describes is not listed on the change record. Perhaps that change record should be updated?
I tested locally and found that only this form worked,
core_version_requirement: ">=8".
The quotes are needed. Also, that "<" works as well.I was thinking something like this which uses examples to show the options.
* - core_version_requirement: Specifies the compatible version or versions of Drupal core. For example, "9.3 || 9.4" means compatibility with Drupal 9.3 and 9.4; ">=9" means compatible with Drupal 9, 10 and later versions, "<=9" mean compatible with Drupal 8 an 9. (Required)
Comment #26
ankithashettyUpdated the patch in #24 with the changes suggested in #25, thanks!
Comment #27
ankithashettySorry, messed up the interdiff file!
Comment #28
hmendes CreditAttribution: hmendes at CI&T commentedThe patch addresses the suggestion from #25 and looks good to me. Changing this to RTBC.
Comment #32
catchI found a couple of typos in #26 but have fixed them on commit. Also update the change record at https://www.drupal.org/node/3070687
Committed/pushed to 10.0.x, cherry-picked to 9.x and 8.x, thanks!