Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Git deploy has 2 roles with upgrade status:
1. It supplies crucial data to categorize projects as contrib and custom.
2. It supplies crucial data to identify versions of contributed projects and thus suggest available updates.
Other than these Upgrade Status does not need or rely on Git Deploy. So one could run upgrade status if we remove the requirement and maybe (some of) their contributed projects would show up as custom but no other harm would be done.
Unfortunately git deploy is causing various problems to users, see #3123286: Fix endless loop when multiple remote branches share base with HEAD where various people reported multiple types of problems.
Proposed resolution
Explore the option to remove this requirement and make it a suggestion instead.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | interdiff-3129903-16.26.txt | 738 bytes | fgm |
| #26 | 3129903-26.patch | 11.28 KB | fgm |
| #18 | 3129903-remove_git_deploy-18.patch | 17.13 KB | fgm |
| #17 | Capture d'écran 2020-04-23 09.58.13.png | 39.37 KB | fgm |
| #15 | 3129903-15.patch | 10.49 KB | Gábor Hojtsy |
Comments
Comment #2
Gábor HojtsyComment #4
Gábor HojtsyWe can (probably should) put more heuristics into upgrade_status, eg. about “modules/contrib” and “modules/custom” to soften the negative effects of not having git_deploy anymore for categorization. This was raised in #3122298: Incorrect category for modules too.
Fixing the fail for now.
Comment #5
Gábor Hojtsy@fgm made some good points on slack, and pointed me to https://www.drupal.org/project/composer_deploy which would actually be a better fit for composer sites. So adding that as a primary recommendation to the readme and to the help text. I am even more convinced that this should be done.
Adding a bit of code to identify contrib projects in
*/contrib/*as contrib. While this is not a mandatory structure, many sites use it, so it works as a best effort fallback. We don't really have other indication as to whether something is contrib or core. Given this modification, we also need collation happen for contributed extensions, and assume a project name for them if they did not have one.I *think* this is functionally complete.
Comment #7
Gábor HojtsyComment #9
Gábor HojtsyOne case forgotten :D
Comment #11
Gábor HojtsyOk understandbly the module now categorizes all test modules under the main project. That is a case of "why/how did it not work like that before".
So skipping "upgrade_status_test_*" modules from our category magic. (Since the category magic depends on path info outside of the control of the test). Also specifying unique projects for the test "projects" under the module.
Comment #13
Gábor HojtsyWe should also skip the collation.
Comment #15
Gábor HojtsyOk got my haystacks and needles wrong with strpos() and also matched the shorter module names instead of longer ones in the collate method. Fixing those two seems to work fine at least for the test scenarios without git_deploy.
Comment #16
Gábor HojtsyAs far as I see that is good. Unless I get complaints will commit tomorrow :)
Comment #17
fgmUpgrade status now works, and recognizes mongodb as a contrib, but four small things:
Patch:
Comment #18
fgmRerolled to fix this issue and a few related others, plus DCS warnings on the modified file.
Note that, although this is likely unrelated, the drush commands fail like:
But in spite of that, the results still display, showing that upgrade_status is not D9-ready at this point:
Comment #19
Gábor HojtsyCan we try to not couple everything all at once to one patch? This makes problems much harder to individually track down :/ Almost half the patch is now unrelated things. I would much rather deal with your concern about After analyzing (not on initial display), it discovers project submodules and lists them as projects too which IS a problem that this patch may be introducing and we need to fix.
The end of line whitespace here remained somehow :D
Comment #20
Gábor HojtsyREQUEST_TIME is not even deprecated for Drupal 9, it is deprecated for Drupal 10 currently: https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/consta...
Comment #21
fgmNot sure about the implications of your last comment: REQUEST_TIME is deprecated since Drupal 8.3 (but removed in Drupal 10), so I figured it would be better to remove it in all code since 8.3 : that's what I did elsewhere. Is this wrong ?
Beyond that deprecation and the PHPCS issues the only actual change I added to your patch is to address the warning about unchecked access to nonexistent data. I did not touch the discovery itself. Do you want me to only include the unchecked access, without the request time and phpcs fixes ?
Comment #22
Gábor HojtsyI am happy to commit the request time and the phpcs fixes in a different patch but not here. We still need to resolve the bug you found here and we should not complicate it with a bunch of unrelated things. Who knows who else will be involved in fixing it, let's keep it streamlined to that IMHO. Thanks!
Comment #23
Gábor HojtsyI reviewed your patch once again and I assume the "unchecked access to nonexistent data". I think this is it:
Would that not be in the realms of #3129430: When phpstan is not found, the error is too subtle, does not support custom binary path? We would need to report the error itself because in this case apparently the output from phpstan was not parse-able. Also this does not seem to be related to removing git_deploy either.
The one thing you reported that I am concerned about is this:
Comment #24
fgmYes, this is the bit I was referring to. But - without debugging it - I don't think it is related to not finding phpstan because errors were reported when there still were some, so I assume it was found.
Regarding the submodules, what is the expected behavior ? Should they be listed from the first GET to the page ? Or not be listed separately even after the scan ?
Comment #25
Gábor HojtsyFor both contrib and custom projects, submodules should be grouped under the topmost extension, see
in the patched code. I think we only ever use our own ProjectCollector to list projects, so I am very surprised they would be different before/after scan. Scanning should not change their project metadata which is the only thing outside ProjectCollector that would change how they are categorized.
Comment #26
fgmProbably good news: I did a reinstall with the latest 8.9.x, phpstan, upgrade_status and mongodb modules, and could not reproduce the issue. And I didn't even get the error. So I guess you don't need to dig further on that part.
However, the errors on the Drush command are still there:
Since other Drush commands not related to phpstan/upgrade_status/drupal-check work normally, this does not seem to be related to the git_deploy removal, but to be the same problem already discovered with drupal-check, so I think it does not belong in this issue.
Now, regarding the json part of my suggested patch, I would still apply it because I like to cover my bases, but that's really your choice. I've attached is the reduced version only containing that bit.
In the web UI (but not with drush us-a or drush us-cs), I still get spurious detection bugs due to the scan not being able to access the class in the mongodb library (a dependency installed in vendor/), but these were already present with git_deploy, so it is an other issue.
I also checked these non-autoloaded classes in the popin with the scan results in
ScanResultFormatter::formatResult()and here they can be found normally, so it's really unrelated and will need to be addressed in a lower layer, during the scan phase.Comment #28
Gábor HojtsyGreat, thank you! I fixed the two one line whitespace problems on commit.
I did not commit the json hardening, but suggested @ndeet that it be taken forward in #3129430: When phpstan is not found, the error is too subtle, does not support custom binary path. I'll try to make sure it is included there. (If you are interested you can join there and help but no obligations :)
Thanks for all your help here!
Comment #29
Gábor HojtsyExtracted the REQUEST_TIME related parts to #3131176: Remove use of deprecated REQUEST_TIME and added one more use of it to fix there.