Reproduce
Install Drupal 8.0.0-beta1 in a language which is supported (tested with Norwegian Bokmål - "nb"). Notice that Drupal downloads drupal-7.0.nb.po (into sites/default/files/translations/) instead of drupal-8.0.0-beta1.nb.po - which exists.
Solution
Make the installer semantic versioning aware.
First attempt for a patch is attached. Changes:
1) Added support for patch level in _install_get_version_info
2) Added support for patch level in install_get_localization_release
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff-36-44.txt | 1.17 KB | zaporylie |
#44 | drupal-installer-semantic-versioning-2349263-44.patch | 13.9 KB | zaporylie |
#36 | interdiff-34-36.txt | 3.54 KB | Désiré |
#36 | drupal-installer-semantic-versioning-2349263-36.patch | 13.97 KB | Désiré |
#34 | drupal-installer-semantic-versioning-2349263-33.patch | 13.96 KB | zaporylie |
Comments
Comment #1
hansfn CreditAttribution: hansfn commentedComment #2
Sutharsan CreditAttribution: Sutharsan commentedFirst pass review. Not yet tested, not checked for missing fallback scenarios due to the extra version level.
Comment line > 80 chars and not ending with a period.
[edit: rephrased]
"core" format does not have a minor. Unchanged at 8.x, etc.
"8.1" should be "8.1.1". "8.x-dev" should be "8.0.x-dev".
Add note that this should be revisited in the 9.0.x branch to fall back to 8.0.0.
See above.
Comment #3
hansfn CreditAttribution: hansfn commentedThx for the comments. I realize now that core number shouldn't have been changed. I'll try to update the patch later today.
Isn't that already handled by the else part of the following code:
Comment #4
hansfn CreditAttribution: hansfn commentedSecond attempt attached. This also handles fall back at patch level. It's also commented slightly better - too much?
PS! I have tested my work with the following little file (test.php):
Running it like "php test.php 8.3.4" and "php test.php 8.0.0-dev".
PS! It seems \Drupal::VERSION is 8.0.0-dev also for the 8.0.0-beta1 release - ref core/lib/Drupal.php Is that correct?
Comment #5
hansfn CreditAttribution: hansfn commentedOK, another too long comment that I didn't spot. (Testing Dreditor.)
Comment #6
hansfn CreditAttribution: hansfn commentedUpdated patch with two too long comments fixed.
Comment #7
Désiré CreditAttribution: Désiré commentedI'm tested and reviewed the last patch, the fix worked for me, the code seems to be ok.
(My patch only fixes some minor coding style issues, so I put this RTBC.)
Comment #8
Désiré CreditAttribution: Désiré commentedThis should have automated tests (or update existing ones).
Comment #10
Désiré CreditAttribution: Désiré commentedComment #11
hansfn CreditAttribution: hansfn commentedSome minor updates to the fall back detection:
1) Simplified logic since we are already using array_unique to remove duplicates.
2) Fixed fallback for minor > 0, patch = 0 which didn't include 8.0.0.
3) Fixed some comments.
Reviewed code:
1) Not in position to say if the change in core/modules/locale/src/Tests/LocaleUpdateInterfaceTest.php is correct.
2) Don't understand why the changes in core/modules/migrate_drupal/src/Tests/d6/MigrateAggregatorFeedTest.php and core/modules/migrate_drupal/src/Tests/d6/MigrateAggregatorItemTest.php are included in the patch.
Sorry about the missing interdiff.
Comment #12
hansfn CreditAttribution: hansfn commentedArg, some ending white space ...
Comment #15
Désiré CreditAttribution: Désiré commentedSorry for that, wrong diff.
Now I'm updated it, included your last changes (from #12), and also changed some comments. The failing test is also fixed.
Interdiff form #10.
They should be correct.
The only question is that we really need fallback for point versions (8.5.4 -> 8.5.3), or it should be enough to fallback to the .0 version (8.5.4 -> 8.5.0)? (Because it's not likely to every point version will have a translation, and even it have after install it can be updated to the freshest easily.)
If we don't need it the code and the test also should be changed.
Comment #16
hansfn CreditAttribution: hansfn commentedI see that the last patch is less intrusive. It doesn't fix all the update.php to core/update.php things. Probably better - keeping this issue clear.
I have reviewed the latest version of the patch and tested it manually and I think were ready to go. (Remove the testing issue tags?)
Yes, we really need that. Imagine when Drupal 8.0.9 is released. If people install it before l.d.o has made a translation package for it, you'll get the translation for 8.0.0 in stead of 8.0.8 - which might be a significant difference. Asking people to redownload the translation immediately after installing isn't user friendly.
My only question is: Isn't there rc/beta/alpha releases for 8.x.0 when X > 0? I mean 8.0.0-dev falls back to 8.0.0-rc1, 8.0.0-beta1, 8.0.0-alpha1 and 7.0 while 8.2.0-dev falls back to 8.1.0 and 7.0. I don't know how the minor release are planned so I can't tell.
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedTypo. Change "introduse" to "introduced".
I have no indication that there will releases like 8.1.1-rc1. Let's base the code on what we currently know and modify it when new information comes up.
Comment line too short.
This also means that for example 8.0.8-dev falls back to 8.0.0. There may be several months between these releases. Let's add a fall back to 8.0.8 too.
There is always room for improvement. I suggest to create a follow-up for this.
I have no information that we will. We can fix it when it comes up, also in a separate issue.
Removing test tags as we have now both manual and automated test coverage.
Comment #21
zaporylieI have small digression into issue priority. I've bumped it up, because I think this is a really major bug. Right now, installator takes really old .po file, with really outdated strings, related to many other, already fixed issues (like #1317884: Remove all instances of <none>, <Hidden> and <br/> from translatable strings because they lead to import errors). I was so confused about it so I started digging into installator and found the reason - missing support for semantic versioning.
Conclusion:
This issue should be fixed and committed asap.
Comment #22
zaporylieComment #23
Sutharsan CreditAttribution: Sutharsan commentedJust talked to Alex Pott on future version numbers. We should anticipate on versions like "8.1.0-rc1"
Comment #24
Sutharsan CreditAttribution: Sutharsan commentedComment #25
Désiré CreditAttribution: Désiré commented'8.2.0-rc2', '8.2.0-rc1', '8.1.0', '8.0.0', '7.0'
. Discussed it with @Sutharsan, '8.1.0' is better there, but not necessary, but the code is also less complicated this way.(I don't know how important or how frequently is this code changing, but it's very ugly and hard to work on it, probably it should be completely refactored in an other issue.)
Comment #26
Sutharsan CreditAttribution: Sutharsan commentedFurther discussed future release versions with Alex Pott in IRC:
alexpott: You confirmed that we may expect 8.1.0-rc* releases. What about beta/alpha for 8.1.0 and onwards? e.g. 8.1.0-beta1 (c.c. DesireJM)
Sutharsan: we'll talk about this in between the committers
Lets see if we can keep this option open in the code, so we do not need to change it again when is gets decided.
Comment #27
Désiré CreditAttribution: Désiré commentedI'm added the fallback for 8.x.0-rc -> 8.x.0-beta1.
There are also some other inconsistent fallback chains, like:
'8.0.0-beta2', '8.0.0-beta1', '7.0'
should include an alpha fallback'8.8.0', '7.0'
, while at least 8.0.0 should be included thereBut the current code is really hard to change, and any new rule could overcomplicate it. So at this point I suggest to commit this when it's possible since the original bug is fixed, and submit a followup issue for make more consistent fallback chains and a refactor for the logic.
Comment #28
Sutharsan CreditAttribution: Sutharsan commentedComment needs to be updated
Add test cases for 8.1.0-beta1 and -beta*
Comment #29
Sutharsan CreditAttribution: Sutharsan commentedDiscussed with Désiré to finalise this issue to handle semantic 8.x.0-dev/beta/alpha releases too. Désiré has ideas for refactoring the code, but we will do that in a separate issue.
Comment #30
hansfn CreditAttribution: hansfn commentedAnother attempt from me. Fixes everything mentioned above and slightly refactored - maybe for the better. All tests still pass.
Comment #31
zaporylieQueued for automated testing.
Comment #32
zaporylie@hansfn - I really like your work! Thanks!
Anyway, I've added a few changes to cover more cases, removed trailing whitespace in tests file, added a sentence to explain that duplicated alternatives will be removed at the end of process.
New case covered:
Comment #34
zaporylieI've forgotten about tests
Comment #35
Sutharsan CreditAttribution: Sutharsan commentedIt was never the intention of this hardcoded version fallback to be complete. It is easy to fall back to the first (rc1, beta1), only with an up to date list of releases (like Update module uses) is needed to get the most recent release. The latter is way out of scope of this issue.
Comment #36
Désiré CreditAttribution: Désiré commentedI'm like #34, it simplifies the logic, and introducing reasonable fallback chains, it also introduce the suggested change from #29. So I'm suggesting RTBC here.
In this patch I'm just changing some coding standard problems (long arrays are in one line again just to match the style in the same file), and adding only one logic change: 8.1.0-foo1 will now fallback to 8.1.0 before 7.0 because it's not to hard to do, while in the translation versions will be huge differences.
Comment #37
zaporylieI'm not sure if this fallback is necessary because every 8.x.0-foo1 should be released before 8.x.0.
Comment #38
Désiré CreditAttribution: Désiré commentedNo, it's an other case: valid extra names (dev, alpha, beta, rc) should be released before the final version release, and all these cases are handled. But 'foo1' here is an example for an invalid extra name, which should not exists. So if something like this happens we should do something, and I think it's better to fallback some fresher version than only to 7.0.
Comment #39
zaporylieYou are absolutely right that we should do something, so...
...so probably we should extend Test above, by adding another one for version 8.3.0-foo2, with fallback path: 8.3.0, 8.2.0, 8.0.0, 7.0 what is covered by your last patch, or extend fallbacks list of fake version with three steps more: 8.3.0, 8.3.0-rc1, 8.3.0-beta1, 8.3.0-alpha1, 8.2.0, 8.0.0, 7.0.
Fallbacks 8.2.0 and 8.0.0 was already covered by #30:
What do you think?
Comment #40
hansfn CreditAttribution: hansfn commentedOK, I have tested and reviewed - again. Some comments:
1) The addition of the two "Duplicates will be removed later." strings doesn't add anything. There is already the string "The fallback detection is relaxed - removing version duplicates at the end." at the top, and if we really want to use "Duplicates will be removed later." everywhere it needs to be added also for the Patch-level comment.
2) Do we really want to spend any more time on invalid releases (foowhatever)? The current fallback is more than sufficient IMHO.
Comment #41
Désiré CreditAttribution: Désiré commentedComment #42
hansfn CreditAttribution: hansfn commentedI notice that Désiré has unassigned himself/herself. Are we just waiting for this to be committed or is there anything else that needs to be done?
Comment #43
zaporylieLater today I'll remove "Duplicates will be removed later" from comments (somehow I've missed "The fallback detection is relaxed..." string), than patch will be ready to commit.
Comment #44
zaporylieOk, here's patch and interdiff
Comment #46
zaporylieHere is interdiff with right, .txt extension.
Comment #47
alexpottCommitted 9deda26 and pushed to 8.0.x. Thanks!
Comment #50
xjm