Follow up #3035337: Port rules.drush.inc to D8

CommentFileSizeAuthor
#13 3075601-13-coding-standards.patch2.39 KBtr
#12 3075601-coding-standards.patch2.31 KBtr
#9 3075601-9-drush9.patch11.1 KBtr
#6 drush9-3075601-5.patch9.53 KBjungle
#2 drush9-3075601-2.patch19.33 KBjungle

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new drush9-3075601-2.patch19.33 KB

Added drupal/typed_data to the composer.json, and removed the composer.lock file.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Active » Needs review
tr’s picture

tr commented

I just committed #3035337: Port rules.drush.inc to D8 so I'm ready to address this!

I think both composer.lock and .travis.yml were committed to the project because @fago was doing his own CI on his GitHub Repository, before the Drupal infrastructure was doing things like coding standards testing and unit testing. @fago was also doing all Rules development on GitHub.

But @fago has not been participating in the Rules project for a long time. Since I came on board as a maintainer last year I've tried to move all development to drupal.org, and I've fixed up the Rules testing configuration and made the Rules tests run green on drupal.org.

Because I don't have access to @fago's GitHub Repository, I can't maintain his CI configuration or administer his pull requests etc. - I have to leave all that up to him, and as I said he's not participating here.

So I have no problem with removing the composer.lock and .travis.yml. phpcs.xml.dist should probably remain, but I'm open to removing any Rules-specific configuration possible and just using the DrupalCI defaults. There have been a LOT of improvements in DrupalCI since these files were last touched, and I don't want to be overriding any of the new DrupalCI stuff unless absolutely necessary.

As far as Drush 9/10 goes - I would like to make sure that we don't remove support for Drush 8 at this time. If we can support both, that would be great. The reason is that Drush now *requires* that the project be installed via Composer, and also that Drush has different PHP requirements than Drupal core. I don't want to impose extra conditions to install Rules above what Drupal core requires.

I am strongly opposed to *requiring* Composer in order to use Drupal/Rules. It is fine to support the use of Composer, but *requiring* it is a different thing - that basically excludes a lot of the non-developer user base as this user base may not have command-line access, may not have a server that meets the ridiculously huge memory requirements of Composer (most shared servers don't, in my experience), and may not have the technical knowledge or *desire* to learn yet another complicated tool just in order to install Drupal.

Rules in particular is most beneficial to the non-developer, but it also gives developers a way to allow non-developers customize site operation. It defeats the purpose if we require Composer, especially since core Drupal does not (yet) require Composer and has made a lot of effort to ensure that Drupal can be used without Composer.

Now about the patch ...

Can you see if the (string) casts can be removed? They were necessary for Drush 8 because dt() returns a TranslatableMarkup object, not a string, and Drush output formatters do not like output containing objects. I hope that problem has gone away in Drush 9. Otherwise, I think it looks good as long as Rules will still install and run without Composer.

tr’s picture

tr commented
Title: Port Drush commands to Drush 9 style » Add support for Drush 9+

New title to more accurately reflect what I think we should be doing.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new drush9-3075601-5.patch9.53 KB

I am strongly opposed to *requiring* Composer in order to use Drupal/Rules. It is fine to support the use of Composer, but *requiring* it is a different thing - that basically excludes a lot of the non-developer user base as this user base may not have command-line access, may not have a server that meets the ridiculously huge memory requirements of Composer (most shared servers don't, in my experience), and may not have the technical knowledge or *desire* to learn yet another complicated tool just in order to install Drupal.

For me as a developer, Rules is much like the views module, it reduces custom code. And, I think composer will be widely used gradually, even there is a WIP initiative #2958021: Proposal: Composer Support in Core initiative.

In order to make the drush9 commands works, the following lines should be added to the composer.json file. It is mandatory.


    "extra": {
        "drush": {
            "services": {
                "drush.services.yml": "^9"
            }
        }
    }

Not sure @fago is happy to remove composer.lock, so let me submit another patch to bring the composer.lock back to see if all tests are green. Do minimal changes as I can.

I would like to make sure that we don't remove support for Drush 8 at this time.

They can co-exist. the Search API module does this way.

Can you see if the (string) casts can be removed?

They can't be removed, without it, the errors below are thrown.

 $ drush rdel demox

Array
 [error]  Object of class Drupal\Core\StringTranslation\TranslationManager could not be converted to string OutputFormatter.php:133 
 [error]  E_RECOVERABLE_ERROR encountered; aborting. To ignore recoverable errors, run again with --no-halt-on-error 
 [warning] Drush command terminated abnormally. Check for an exit() in your Drupal site.
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

BTW, indention of composer.json is adjusted to meet the coding standard from 2 spaces to 4 spaces.

tr’s picture

tr commented
Related issues: +#2912821: Improve compatibility with composer-based workflows

I think the above patch also solves #2912821: Improve compatibility with composer-based workflows

tr’s picture

tr commented
StatusFileSize
new 3075601-9-drush9.patch11.1 KB

  • TR committed 7cbc72f on 8.x-3.x 
    Issue #3075601 by TR, jungle: Add support for Drush 9+
tr’s picture

tr commented
Status: Needs review » Fixed

Committed.

tr’s picture

tr commented
Status: Fixed » Needs review
StatusFileSize
new 3075601-coding-standards.patch2.31 KB

Ah, missed a few coding standards ...

tr’s picture

tr commented
StatusFileSize
new 3075601-13-coding-standards.patch2.39 KB

And the remaining one is a false positive. It says "unused use statement" but we really do need that use statement, so let's hide it.

  • TR committed a9e00e7 on 8.x-3.x 
    Follow up to Issue #3075601: Coding standards.
tr’s picture

tr commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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