Problem/Motivation
Now that #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches has been done core_version_requirment
can be used to declare core compatibility via Composer semver constraint. It is technically already possible to declare that your module is already compatible with Drupal 9 only. For instance if a module requires one of the newly updated composer dependencies in Drupal 9.
This means that currently the update module could be listing contrib modules that are only compatible with Drupal 9. Previously this would not have been possible.
Once #3009338: [META] Support semantic versioning for extensions (modules, themes, etc) in Drupal core, and allow modules to be compatible with Drupal 8 and 9 at the same time is completed there will no correlation between the module version number and core compatiblity.
Therefore we should display the core compatibility of updates to display this information to the user.
In #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core we determined we want
- Should a message on all update that display their core compatibility based of the
core_version_requirement
in info.yml files. This information is now in the update XML from drupal.org ascore_compatibility
The message should be in this format:
This module is compatible with Drupal core: 8.8.3 to 8.9.1
- As 2nd step will switch to a update XML feed that is not tied to CORE_COMPATIBILITY but rather all 8.x and future module updates.
This will be handled in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates
Original all this handled in #3074993 because only the /current
feed had core_compatibility
but now this available in the current XML we can handle this first in it's own issue.
Proposed resolution
- Done: in #3074998: Add explicit information about core compatibility to update data add a new
core_compatiblity
value for each update. This is will be based oncore_version_requirement
or if not available it will be basedcore: 8.x
This is will be for the project not individual modules in the project. Details to be determine in that issue
- Display a message for each available project update that displays the core compatiblity range based on 1) actual available updates the update module has retrieved from the update server and 2) the
core_compatiblity
for the project update xml(see above) - For most common possibilities for
core_compatiblity
this will be able to displayed as a simple range.For example
Installed core: 8.8.3
latest 8.x patch release: 8.9.1
Drupal 9 not released yet
project xmlcore_compatiblity: ^8 || ^9
This module is compatible with Drupal core: 8.8.3 to 8.9.1
If Drupal 9.0.3 had been released this would change to
This module is compatible with Drupal core: 8.8.3 to 9.0.3
if project xml was
core_compatiblity: ^: ^8.8.9 || ^9
(not usingcore_version_requirement
because the value will be directly from the project xml not the info.yml files)
This module is compatible with Drupal core: 8.8.9 to 9.0.3
- Some less common values for
core_compatiblity
will not be able to display by a simple range. In that case we should display them in a series of ranges.For example
Given:
Installed core: 8.8.3
latest 8.8.x: 8.8.7
latest 8.x patch release: 8.9.1
Drupal 9 not released yetcore_compatiblity: ~8.8.0 || ^9
This module is compatible with Drupal core: 8.8.3 to 8.8.7
Because 9 hasn't been released it won't come into play.
If Drupal 9.0.3 had been released
This module is compatible with Drupal core: 8.8.3 to 8.8.7 and 9.0.0 to 9.03
- In this first step no modules will be filtered out of the list.
This will mean hypothetically you could get projects updates with the message like
This module is compatible with Drupal core: 9.0.0 to 9.0.5
In the near term this is very unlikely because 1) Drupal 9 is not out so no module should be compatible with it but not Drupal 8 and 2) 9.0.0 should be compatible with 8.9.0 for modules not using deprecated code. Therefore any module that is compatible with 9.0.0 because it remove deprecations should also be compatible with 8.9.0. So the project would likely have this in the project xml
core_compatiblity: ^8.9 || ^9
orcore_compatiblity: ^8.8 || ^9
Remaining tasks
Finish patch, review
User interface changes
New messages on available updates list
API changes
none
Data model changes
None
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#48 | 3096078-48-8.8.patch | 24.35 KB | tedbow |
#39 | s-narrow3.png | 88.71 KB | tedbow |
#39 | s-wide1.png | 90.81 KB | tedbow |
#36 | setReleaseMessage_ide.png | 80.56 KB | tedbow |
#35 | 3096078.31_35.interdiff.txt | 3.73 KB | dww |
Comments
Comment #2
tedbowHere patch with tests. I need to clean the code to standards still so leaving assigned to myself.
Comment #3
tedbowNope here is patch
Comment #4
tedbowJust a couple thoughts/questions. The patch is still rough so no need for a nit review yet.
Originally this class was just static methods. I changed it to have constructor because where the core info is set because that will not change per project.
Currently in the ranges the patch is not taking into account non stable core versions
So if current version is 8.0.0, and there are updates 8.0.1, 8.0.2 and 8.1.0-alpha1 it will display the message
should this be
If someone has opinions on what the markup should be that would be great.
I no longer need to set this.
I move the 'drupal' project to the beginning of the array so that it can calculated first. Before setting other module ranges we have to make sure core is set.
The other option would just be to call
update_calculate_project_update_status($projects['drupal'], $available['drupal']);
before the loop below and just skip it in the loop.I am thinking now that would be better.
Comment #5
tedbowI shouldn't have actually set
core_compatiblity_ranges
in the release array. Just the message is used later.If we put this array it will end up being "array as api" ☹️
So let's just set the message.
This allows keeping class level cached messages instead of the ranges so refactoring some other stuff in this class.
This actually a change I didn't up needing because I not using this test file. Reverting.
Comment #7
tedbowSome general clean and the items below. Read for review I think.
Removing this inline comment and replacing them with a better
@param
doc for$project_data
that documents what the keys of the array that this method will use.This logic around
$previous_version_satisfied
is not longer needed. RemovedThis part of the message also needs to be translated. fixed
$available['drupal']['releases']
is not set in all the test XML which caused the fails. This could happen for real I guess so will check firstComment #8
Gábor HojtsyThe patch looks good. I read the issue summary. It is not clear why do we need to add this new messaging though, that is not explained.
Also I don't think a Critical task is a valid combination.
Comment #9
tedbow@Gábor Hojtsy ok thanks. I update the summary to list why need this.
I marked this has critical because @xjm marked #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core as critical and this implements it's first step. Also in that issue @catch said these changes should get in even during 8.8.0-rc phase. So that would mean we have very little time
Comment #10
bnjmnmAdd a comma after "If set"
Ultranit: "the" in the second line can be moved to the first.
Is it necessary for
$security_update
to be passed by reference since it's not being changed, just added to an array?Ultranit: pluralize "element"
Ultranit: add comma after "range"
I feel like there there'd be benefit in adding this to Stable so it's visible to 8.x users in everyday use. Adding a span -seems- acceptable, but I looked through Stable's commit history for the past 2 years and couldn't find any firm precedent. The closest was https://www.drupal.org/node/77245, where a wrapper was added to messages.
This comment seems incomplete.
These need types and comments and
$release_title
needs to be added to meet coding standardsUltranit: make Dataprovider two words.
Comment #11
tedbow@bnjmnm thanks for the review
$project_data
but in both cases we want to check if$release['core_compatibility']
and then set the message if it is. Having 1 array,$releases_to_set
that is an array of references to the releases allows us to have 1 loop that sets messagesRight now there is test coverage if the loop doesn't set by reference
\Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage()
will fail. But right now\Drupal\Tests\update\Unit\ProjectCoreCompatibilityTest::testSetProjectCoreCompatibilityRanges()
won't fail. Since the unit test provide a more clear documenting of\Drupal\update\ProjectCoreCompatibility::setReleaseMessage()
's functionality I will add test coverage there too. It just involves moving 1 of the releases under the expected project data tosecurity updates
.$defaultTheme
to see how this looked in Bartik and the test failed ☹️Looks like stable and claro override this template so these will have to updated also. but we can't update claro correct? Anyways I am not addressing it in this patch.
I don't think we actually need to test the link href here because that is tested in other methods. This makes
testCoreCompatibilityMessage()
andassertCoreCompatibilityMessage()
both simpler.changed
$release_label
to$version
and the last 2 parameters to$expected_compatibility_range, $expected_release_title
.Comment #12
lauriiiWe should definitely add this to Stable.
I think we should add this to Stable too to make sure that this feature can be used in as many themes as possible. Theoretically, this could cause regressions on a heavily customized version of the page but it still seems like a better idea to add this to provide the feature to our users. I'm wondering if we actually need the span given that we are not adding any classes to it?
We should also update Claro 😇
Comment #13
tedbowComment #14
bnjmnmOne tiny nit and that should do it
This should be
latest_version
Comment #15
tedbowfixed
Comment #16
bnjmnmNo more nits to be found, RTBC.
Comment #17
lauriii+1 to adding the HTML class here because this template doesn't have Classy equivalent and it's more consistent with the rest of the template. Opened a follow-up to clean up that: #3100204: Remove visual CSS classes from update-version.html.twig.
Comment #19
bnjmnmSwitching back to RTBC, the fail was in an unrelated test of the unpredictable-FunctionalJavascript variety.
Comment #20
Gábor HojtsyReviews as well. Looks good to me, other than the following :)
While t() is not deprecated, as per its docs:
Also, the '@start to @end' string would be very little to provide context for translators as to what it means. Is it a distance or dates or percentages? It may need different translations based on that. I would suggest to change it to '@low_version_number to @high_version_number' or somesuch. Alternatively you can add an explicit context to the string but IMHO once we are specific enough with the placeholders, like @low_version_number, it will not need more context.
Comment #21
tedbow@Gábor Hojtsy good catch. Yes I think was using StringTranslationTrait but ran into a problem with the unit test.
Didn't know I could just do
Fixed. and used your suggested translation placeholders. Much clearer.
Comment #22
Gábor HojtsyShould be changed in the replacement array as well :D
Comment #23
tedbow🤦♂️ I ran the unit test locally after I switched to the trait but not after I update the placeholder 😞
fixed
Comment #24
bnjmnmConfirmed the feedback from @Gábor Hojtsy was addressed.
Comment #27
Gábor HojtsySuperb, thanks for fixing that. No other complaints, and this is a fundamentally important improvement. Thanks a lot!
Comment #28
dwwSuper exciting to see this so far along! Mostly looks really good. I did *not* yet closely review all the test coverage. However, in the rest of the patch, I found some nits, and a few concerns of substance:
Would love to have a little more substance in this comment. This class is doing some fairly complicated things, and it'd be nice to get an overview without having to read the whole thing...
Comment doesn't match the class name.
Nit: We should be consistent with capitalization of "Drupal core".
If there's no "existing_version", this whole class is meaningless, right? Shouldn't this throw an exception instead of quietly being a no-op class?
Don't we mean "The currently installed version of Drupal core" or something? Wouldn't this be better as
$installed_core_version
or so?"core compatibility of based on core" doesn't parse. Comment seems to be missing some word(s).
Also, see above about if this should be throwing an exception instead of silently becoming a no-op class.
Maybe add @see comments at the end of this to make it easier to find where all these values are documented? "A project version number" on its own isn't particularly illuminating. But we don't necessarily want to re-document what "recommend" vs. "latest_version" vs. "also" actually means. So maybe @see as a compromise?
To the untrained eye, it's not clear what "Composer semantic version constraint" means here. What's Composer-y about this? It's that we're using composer-esque constraint syntax, right? Is there a @see we should use as a reference? Or can we drop the Composer(tm) from this? Or better explain what we mean?
While we're considering translatability of this code, isn't this line impossible to properly translate in Arabic or another RTL language?
Don't we need something like:
?
The last point is the only thing that's truly NW-worth of this review. Everything else is mostly comments and cosmetic nits. But I don't see how RTL languages can make this work if it's hard-coded to have
Text . ' ' . stuff
ordered in the code.p.s. Ugh, @Gábor Hojtsy already committed this while I was writing this comment. :(
Comment #29
dwwp.s. Is this a candidate for an 8.8.x backport? Wasn't that something @catch said about trying to get this round of update.module improvements in ASAP? I know that'd break normal commit strategy, but this is a bit special-case-y. Although with new strings to translate, it's a bit more disruptive to backport into 8.8.1 at this point.
Comment #30
Gábor HojtsyJust briefly documenting the discussion we had with @dww on slack. The lack of use of placeholder is not an RTL concern, the source order in LTR and RTL is the reading order and thus the same. The source order does not need to be in a different direction for RTL languages, the browser will display the content in a different direction from the source order though for RTL content.
Having a placeholder approach would be somewhat better for cases where you need to translate in a way that the version list is within a sentence, but Drupal is generally oriented to a Field name: value kind of display already, so I did not consider this a deal breaker. Doing that would allow for a translation in the style of This module is compatible with 8.8.0 to 8.9.3, 8.9.5 to 8.9.9, 9.0.0 to 9.0.10 versions of Drupal core, but making it a sentence would likely break scannability of the UI in such a translated case.
Comment #31
dwwThis fixes most of #28:
1. Fixed.
2. Fixed.
3. Fixed.
4. TBD
5. Hah! The "existing" terminology is from code I wrote a bazillion years ago in other parts up update.module and friends. ;) Leaving this mostly as-is, slightly improving the doc comment.
6. Fixed the comment. No exception thrown, pending #4.
7. Fixed.
8. Fixed: I removed 'Composer' from these comments.
9. Untouched. Per Slack discussion and #30 from @Gábor Hojtsy - I'm wrong and this actually does work for RTL. I confess to still not totally understanding why. ;) But if anyone would know, it's @Gábor Hojtsy, so I'll defer to his wisdom on this. ;)
Comment #32
lauriiiNitpick: Let's remove the class name from here. It doesn't provide any insights to the reader and it's hard to keep up to date.
Comment #33
tedbow@dww thanks for updated comments. Most look good to me just some thoughts on the class comment
My preference would be that we don't document the internal workings of the class such as the protected member in the class doc.
The updated description on
$possibleCoreUpdateVersions
seems like a good idea I don't think it also needs to be explained in the class doc. For a consumer of the class it isn't important and just is more to read to figure out how to use the class. Also from a maintainability standpoint we also have to update the class doc if the internals change.Again I don't think explaining the internal functioning of the class belongs in class doc. I think if this needs more explaining it should either be inline where we check
or in the
$compatibilityMessages
commentI feel like this description would be more useful where
\Drupal\update\ProjectCoreCompatibility::setReleaseMessage()
is called inupdate_calculate_project_data()
rather than here with maybe a @see to that functionThis connects it directly to the available updates report and if we were to use this someone else in the future we would also have to add the use case of the new place we called it.
update_calculate_project_update_status
which also will not function if this is not set. Or maybe we could handle that in #3100110: Convert update_calculate_project_update_status() into a classAnother option for the current class might be to leave these lines the way it is but in
\Drupal\update\ProjectCoreCompatibility::createMessageFromCoreCompatibility()
ifexisting_version
is empty set the message to be something like "Core compatiblity could not be determined because unable to determine installed version"For instance most of
\Drupal\update\ProjectCoreCompatibility::setReleaseMessage()
is logic to determine which releases will actually be shown on the report. I could see where we might need this somewhere else and want to take this logic out of this class.Comment #34
dww@lauriii re: #32 -- I think it's generally pointless to have a docblock for constructors at all. ;) But core seems to say "Constructs a X object." everywhere. If we want to stop doing that, seems we should change our standards and do it consistently and update everything. Special-casing single classes where we say "Constructs the object." (what else would it say?) seems a little weird at this point. Can you clarify?
@tedbow re: #33 -- Sure, makes sense. I tend to fall on the side of verbose comments, but yeah, you're probably right on all 5 points. I do think the class docblock needs more than the 1-liner ("Utility class to set core compatibility messages for module updates."), but yeah, we should move the explanation of the internals closer to where it's all happening.
Also:
While we're re-writing this and moving this text around, we should fix the fact I didn't close the parenthesis. ;)
I'll take another stab at this. Stay tuned.
Comment #35
dwwFixes (I think) everything from #33 except point 4 (see also #28.4). I'm still not sure the right approach for that. Honestly, this seems like
assert()
territory to me, since I don't know how update.module can do anything if it can't figure out your current version of core. ;) But that's probably better handled in a follow-up. I guess leaving it empty/no-op is best. If you *were* on some bizarrely unknown core version, I'd rather the available updates stayed relatively quiet, instead of repeating "Core compatibility could not be determined because unable to determine installed version" next to every single release.Speaking of which... did we do any usability review of this change? I see no screenshots. Having only looked at the code and not tested on a real site, it seems like we might be duplicating a *lot* of text on the available updates report now. I don't see any discussion of that above, nor any attempts to minimize the noise or make it less distracting.
Tempted to retroactively tag this for "Needs usability review".
Comment #36
tedbowI still think it is more helpful to have a shorter class doc. This describes
setReleaseMessage()
but is not on that method so any automated tool a developer where to use to see the doc for that method would not have this text.I use PHPStorm but I think this would be true for any IDE that lets you view the docs when looking at a method call.
above doesn't have this helpful description.
This would also be true from api.drupal.org. Example method with longer description: https://api.drupal.org/api/drupal/core%21modules%21update%21src%21Update...
Just check and we use the the term "administrator(s)" much for often in core comments than we do "site builder". I think it would be good to change to that.
Comment #37
Gábor HojtsySince this followup discussion is now going on for almost a week, it does not seem like the quick fix it was meant to be. Since it does not seem to concern the working parts of the patch, I don't see a reason to roll this back. It would therefore be more useful to open a followup for the fixes. Otherwise the additional work here messes with issue/commit credit data. The current patch is not a critical task, while the one committed was, etc.
Opened #3101547: Clean up code documentation for display of core compatibility ranges for available module updates and transferred the patch and credits.
Comment #38
dwwFair enough. I was thinking of doing the same, once I realized the RTL thing was a false alarm.
However what about this from #35?
I'm still not convinced this patch should have been committed in the state it was. We missed 8.8.0. So there's not really a rush...
Thanks,
-Derek
Comment #39
tedbowI don't think we should rush things but the importance of this issue is that it blocks #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates which is the last blocking issue here: #2608062: [META] Requirements for tagging Drupal 9.0.0-alpha1
I personally don't think the state was that bad. I think #3101547: Clean up code documentation for display of core compatibility ranges for available module updates will add better docs but I don't the docs were expectionaly lacking in the commit. Most of the stuff I see in is #3101547 is more explanation not doc standards problems or missing docs.
In hindsight I think we could have a usability review but again I think that could probably be said for most issues that add text to the page to any part of core. We should get better with that but I don't think it is reason to revert this issue.
Here are 2 screenshots of the tests, 1 wide screen, 1 narrow. I don't think there are issues that couldn't be addressed in a follow-up
Right now there is a bug on drupal.org with the XML so we can't do a screenshot with live XML. https://www.drupal.org/project/drupalorg/issues/3074998#comment-13395145 . . It is fixed but will take ~8hrs to regenerate the XML.
Yes, there will be text duplicated on the report but that is because many modules will have the same compatibility. I think we should prefer robustness here especially because after #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates we will have Drupal 8 listing Drupal 9 only updates and Drupal 9 listing Drupal 8 only modules. Again we can actually make the filtering better but you for the foreseeable you will see updates that aren't compatible with your current installed core version.
Comment #40
dwwRe #39: thanks for the replies!
39.1: Ok, makes sense. I didn't realize this was the last blocker. Agreed.
39.2: Sorry that came across harsh. You're right, this wasn't in that bad shape. It's just that many other core patches are stopped for more trivial things than UX review + documentation. #3101547: Clean up code documentation for display of core compatibility ranges for available module updates is definitely not critical.
39.3: Agreed, moved to another child issue: #3102724: Improve usability of core compatibility ranges on available updates report Not sure if/where to add that as a "should have". ;)
Cheers,
-Derek
Comment #41
tedbowNo worries. Thanks for enagining in this issue and opening the follow-up
Comment #43
xjmWe're discussing backporting this to 8.8.x, which makes sense (even though it's a string addition and a UI change) because 8.8 users will need this info too until 8.8's end of support in December 2020.
However, I think this issue should probably have had a usability review before going in. I'd like to do that before backporting since we can refine it in as many issues as we want in 8.9/9.0, but once it's in 8.8 any further improvements would be a string break. So reopening as NR/Needs usability review and pinging someone who may have thoughts. :)
Comment #44
xjmI missed #3102724: Improve usability of core compatibility ranges on available updates report, but let's apply the usability gate before commit next time. :)
Comment #45
xjmUntagging and postponing the backport on #3102724: Improve usability of core compatibility ranges on available updates report.
Comment #46
tedbowCreated another follow up for a bug #3112962: Core compatibility messages on contrib available updates don't consider supported branches
The situation that causes it is very unlikely in core releases
Comment #47
tedbow#3102724: Improve usability of core compatibility ranges on available updates report has been committed.
So not sure how this should work should this be committed to 8.8.x and then immediately #3102724 or a combined patch?
Comment #48
tedbow8.8.x reroll
Comment #49
tedbowYay passing against 8.8.x
Comment #51
Gábor HojtsyThanks, committed this to 8.8.x, so we #3102724: Improve usability of core compatibility ranges on available updates report can be rolled against 8.8 as well. Better to use the same issue scopes and not intermix them.