Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nrackleff created an issue. See original summary.

Gábor Hojtsy’s picture

Status: Active » Postponed

First of all, the module is based on the components of https://github.com/mglaman/drupal-check and that is a perfectly fine command line tool. I understand installing both could look superfluous, but that one can be installed without interfering with your site in any way, so that is more versatile in that sense. It is true that Upgrade Status has some useful addon features (eg. categorizing found issues), but I hope those will land back in drupal-check as well.

That said, it is not impossible to make Upgrade Status expose a drush command. I think that would be possible after #3053090: Add an ASCII export option since that would be basically the output formatter. I welcome help in starting the drush command code in the meantime though and/or help with that issue. (I think @tsegat is looking at that one, but not sure).

nrackleff’s picture

Thank you Gábor. We will try the drupal-check command line tool as well.

tsega’s picture

Gábor Hojtsy’s picture

Title: Drush option? » Add drush command to Upgrade Status
Gábor Hojtsy’s picture

Status: Postponed » Active
rpsu’s picture

Version: 8.x-1.0-beta1 » 8.x-1.x-dev
FileSize
1.97 KB

Here is a start, analyse is executed but the output handling is not yet very nice table format.

rpsu’s picture

Drush\Drupal\Commands\core\DrupalCommands has a better solution, maybe.

Gábor Hojtsy’s picture

Status: Active » Needs work

@rpsu: I think you mixed this up, and did not want to upload the readme patch?

rpsu’s picture

You are absolutely right.

Perhaps this patch is better suited for this issue? The table is not printing out nicely for some reason (drush us-a upgrade_status as an example, results table breaks). We are here also re-using the asciiFormat -build, but that may be the cause of the misalignments.

rpsu’s picture

the patch contains also some changes in composer.json - nette/neon was not present even if used, commit 667446f0 in April.
I also tagged drupal/git_deploy with a recent release in favor of using a tag instead of HEAD.

rpsu’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
9.69 KB

Now the output is handled better, and re-using the ascii output is dropped. Output is kind of verbose, but I think it is better this way compared to making it very concise.

# drush -v  us-a role_expose upgrade_status thunder_base
Starting the analysis. This may take a while.
 [notice] Extension role_expose contains 7 files to process.
 [notice] Extension upgrade_status contains 18 files to process.
 [notice] Extension thunder_base contains 0 files to process.
================================================================================
========================== Project(s) of type module: ==========================
================================================================================

Role Expose,  8.x-1.1+1-dev
Scanned on Sat, 08/31/2019 - 02:35

No known issues found.

================================================================================
Upgrade Status,  8.x-1.0-beta3+5-dev
Scanned on Sat, 08/31/2019 - 02:35

FILE: modules/contrib/upgrade_status/src/DeprecationAnalyser.php

STATUS        LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix later     326  Call to deprecated function file_prepare_directory().
                   Deprecated in Drupal 8.7.0, will be removed before Drupal
                   9.0.0. Use
                   \Drupal\Core\File\FileSystemInterface::prepareDirectory().

FILE: modules/contrib/upgrade_status/src/DeprecationAnalyser.php

STATUS        LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix later     334  Call to deprecated function file_prepare_directory().
                   Deprecated in Drupal 8.7.0, will be removed before Drupal
                   9.0.0. Use
                   \Drupal\Core\File\FileSystemInterface::prepareDirectory().
--------------------------------------------------------------------------------

FILE:
modules/contrib/upgrade_status/tests/modules/upgrade_status_test_contrib_error/s
rc/Controller/UpgradeStatusTestContribErrorController.php

STATUS        LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now       15   Call to deprecated function format_string(). Deprecated in
                   Drupal 8.0.0, will be removed before Drupal 9.0.0. Use
                   \Drupal\Component\Render\FormattableMarkup.
--------------------------------------------------------------------------------

FILE:
modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/fatal.php

STATUS        LINE                           MESSAGE
--------------------------------------------------------------------------------
              3    Syntax error, unexpected T_STRING on line 3
--------------------------------------------------------------------------------

FILE:
modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/src/Contr
oller/UpgradeStatusTestErrorController.php

STATUS        LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now       10   Call to deprecated function menu_cache_clear_all().
                   Deprecated in Drupal 8.6.0, will be removed before Drupal
                   9.0.0. Use \Drupal::cache('menu')->invalidateAll() instead.
--------------------------------------------------------------------------------

See #3056118: Keep Upgrade Status itself Drupal 9 compatible other than
testing code [1]


[1] https://drupal.org/project/upgrade_status/issues/3056118


================================================================================
========================== Project(s) of type theme: ===========================
================================================================================

Thunder Base, --
Scanned on Sat, 08/31/2019 - 02:35

No known issues found.

Status: Needs review » Needs work

The last submitted patch, 12: upgrade_status-add-drush-command-3067415-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rpsu’s picture

Assigned: Unassigned » rpsu
Status: Needs work » Needs review
FileSize
10.08 KB
7.1 KB

Refactoring linter issues away (I hope), but I have no clue why UI tests started to fail with this. Fingers crosses with this one - locally tests are green and linter happy.

Status: Needs review » Needs work

The last submitted patch, 14: upgrade_status-add-drush-command-3067415-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rpsu’s picture

Assigned: rpsu » Unassigned

One more small linter issue, but tests will be still failing. I'm a bit puzzled why, since looks like tests are not able to find upgrade_status -module itself in tests -section.

rpsu’s picture

rpsu’s picture

Status: Needs work » Needs review

trigger the tests again

Status: Needs review » Needs work

The last submitted patch, 17: upgrade_status-add-drush-command-3067415-16.patch, failed testing. View results

rpsu’s picture

Component: User interface » Code
Status: Needs work » Active

So all tests pass when I run them locally, but failed with CI - alas tests and all code that is tested are untouched.

rpsu’s picture

This is the output on my local:

$  ../../vendor/bin/phpunit --group upgrade_status
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing 
......                                                              6 / 6 (100%)

Time: 27.61 minutes, Memory: 828.50MB

OK (6 tests, 121 assertions)

Remaining deprecation notices (6)

  4x: file_prepare_directory() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::prepareDirectory(). See https://www.drupal.org/node/3006851.
    2x in UpgradeStatusUiTest::testUiAfterScan from Drupal\Tests\upgrade_status\Functional
    2x in UpgradeStatusAnalyseTest::testAnalyser from Drupal\Tests\upgrade_status\Functional

  2x: Service '0': option 'class' should be changed to 'factory'.
    1x in UpgradeStatusUiTest::testUiAfterScan from Drupal\Tests\upgrade_status\Functional
    1x in UpgradeStatusAnalyseTest::testAnalyser from Drupal\Tests\upgrade_status\Functional

Gábor Hojtsy’s picture

Status: Active » Needs work
jefuri’s picture

I got an error from the last patch in #16, but fixed it in this patch. InvalidArgumentException use statement was not correct.
Also added the possibility to scan everything if no projects are given as an argument.

jefuri’s picture

Status: Needs work » Needs review
tim-diels’s picture

The last patch is not correct. I use the patch in #16 with correcting the use statement for InvalidArgumentException to \ InvalidArgumentException

This patch does not include the extra "Also added the possibility to scan everything if no projects are given as an argument."

tim-diels’s picture

Also fixed the invalid class call in the annotation.

Gábor Hojtsy’s picture

FileSize
9.31 KB

Rerolled and cleaned up the user facing portions of it for a start. I made it compatible with Drush 9 or 10. I have 10 locally and it registered with it fine. I did not manage to run it due to Twig parsing errors that I think are unrelated:

$ drush upgrade_status:analyze upgrade_status

[error]  Error: Call to a member function setParser() on array in Twig\Parser->parse() (line 99 of /vendor/twig/twig/src/Parser.php) #0 /Users/gabor/.composer/global/drush/drush/vendor/twig/twig/src/Environment.php(563): Twig\Parser->parse(Object(Twig\TokenStream))
#1 /vendor/twig/twig/src/Util/DeprecationCollector.php(67): Twig\Environment->parse(Object(Twig\TokenStream))
#2 /vendor/twig/twig/src/Util/DeprecationCollector.php(49): Twig\Util\DeprecationCollector->collect(Object(Twig\Util\TemplateDirIterator))
#3 /web/modules/contrib/upgrade_status/src/DeprecationAnalyzer.php(261): Twig\Util\DeprecationCollector->collectDir('modules/contrib...', '.html.twig')
#4 /web/modules/contrib/upgrade_status/src/DeprecationAnalyzer.php(166): Drupal\upgrade_status\DeprecationAnalyzer->analyzeTwigTemplates('modules/contrib...')
#5 /web/modules/contrib/upgrade_status/src/Commands/UpgradeStatusCommands.php(136): Drupal\upgrade_status\DeprecationAnalyzer->analyze(Object(Drupal\Core\Extension\Extension))

The patch will not succeed here until the composer parts are added, but that I believe would start failing on sites that have drush without the actual drush services / command file in place, I am reluctant to commit it in the interim.

  • Gábor Hojtsy committed 7052475 on 8.x-2.x
    Issue #3067415 by rpsu, tim-diels, Gábor Hojtsy, jefuri: Add drush...
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Making a bit more tight:

$ drush upgrade_status:analyze upgrade_rector
Starting the analysis. This may take a while.
 [notice] Processing /Applications/MAMP/htdocs/web/modules/contrib/upgrade_rector.
 5/5 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


================================================================================
Upgrade Rector,  8.x-1.x-dev
Scanned on Mon, 01/20/2020 - 19:14

FILE: modules/contrib/upgrade_rector/src/Form/UpgradeRectorForm.php

STATUS        LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Fix later     204  Call to deprecated function file_directory_temp().          
                   Deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
                   Use                                                         
                   \Drupal\Core\File\FileSystemInterface::getTempDirectory()   
                   instead.                                                    
--------------------------------------------------------------------------------
Fix now       366  Call to deprecated constant LOCALE_PLURAL_DELIMITER:        
                   Deprecated in drupal:8.0.0 and is removed from drupal:9.0.0.
                   Use Drupal\Component\Gettext\PoItem::DELIMITER instead.     
--------------------------------------------------------------------------------
Fix now       367  Call to deprecated function drupal_set_message(). Deprecated
                   in drupal:8.5.0 and is removed from drupal:9.0.0. Use       
                   \Drupal\Core\Messenger\MessengerInterface::addMessage()     
                   instead.                                                    
--------------------------------------------------------------------------------

FILE: upgrade_rector.info.yml

STATUS        LINE                           MESSAGE                           
--------------------------------------------------------------------------------
              0    Add <code>core_version_requirement: ^8 || ^9 to      
                   upgrade_rector.info.yml to designate that the module is     
                   compatible with Drupal 9. See                               
                   https://www.drupal.org/node/3070687.                        
--------------------------------------------------------------------------------

Still fails for me with the same Twig error as above for upgrade_status itself but will count that up to some local weirdness as I asked Lauriii to reproduce and he could not.

@jefuri: I committed the patch for now. Can you look into re-implementing your suggestion to scan all projects in a followup? That sounds like a useful thing in itself.

Gábor Hojtsy’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.