Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add drupal check dependency #181

Merged
merged 6 commits into from
Mar 11, 2022
Merged

Add drupal check dependency #181

merged 6 commits into from
Mar 11, 2022

Conversation

becw
Copy link
Member

@becw becw commented Jan 7, 2022

Add mglaman/drupal-check as a dependency, since Phing dependencies no longer conflict with drupal-check dependencies. This means that we no longer have to do gymnastics around finding the location of the drupal-check bin on whatever system we're running on (localhost, ddev, vagrant, circleci).

To test:

composer create-project palantirnet/drupal-skeleton example dev-develop --no-interaction
cd example
composer require --dev palantirnet/the-build:dev-add-drupal-check-dependency
vendor/bin/the-build-installer
vendor/bin/phing code-review

@becw becw requested review from byrond and eric-schmidt January 7, 2022 20:24
@becw becw force-pushed the add-drupal-check-dependency branch from 37229f7 to f34f69c Compare January 7, 2022 20:36
Copy link

@eric-schmidt eric-schmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@becw Thanks for those updated notes -- that all makes sense! Unfortunately, when I run through the testing instructions, I'm seeing the following errors:

Screen Shot 2022-01-21 at 2 38 02 PM

That seems to relate to this command: <exec command="${drupal-check.command}" logoutput="true" checkreturn="true" />, but it also seems to relate to some bad formatting that drupal-check itself doesn't expect. Any ideas?

composer.json Show resolved Hide resolved
@byrond
Copy link
Contributor

byrond commented Jan 25, 2022

@becw I'm trying this on the project I am currently resourced to and will report back. I am going to try with require-dev since we'll only use it on CircleCI and local development.

@byrond
Copy link
Contributor

byrond commented Jan 26, 2022

This is working on my current project. There, I updated the CircleCI config, build config, and build.xml. I added drupal-check to the project's dev dependencies with Composer, and it is working fine there. Once this PR is merged, and we have a new release, I can update it in the project, then remove the explicit dependency from the project, as well as the lines from the build config.

@byrond
Copy link
Contributor

byrond commented Jan 26, 2022

@becw I also ran through the testing steps for a new project and didn't have the same error as @eric-schmidt.

example > drupal-check:

     [echo] $> vendor/bin/drupal-check docroot/modules/custom/
    0 [▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░░]
     [exec] 
     [exec] 
     [exec]  [OK] No errors
     [exec] 

example > drupal-check:

     [echo] $> vendor/bin/drupal-check docroot/themes/custom/
    0 [▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░░]
     [exec] 
     [exec] 
     [exec]  [OK] No errors
     [exec] 

BUILD FINISHED

Total time: 5.4969 seconds

@eric-schmidt
Copy link

This appears to be an issue with drupal-check itself, as I get the same error when manually running it outside of phing. What is weird is that I can run the exact same command using the exact same version of drupal-check on other sites w/o issue 🤔

Screen Shot 2022-01-28 at 1 29 12 PM

@byrond
Copy link
Contributor

byrond commented Jan 31, 2022

@eric-schmidt what version of PHP are you using on your host?

@byrond
Copy link
Contributor

byrond commented Jan 31, 2022

@eric-schmidt @becw I added this to an existing project, and although I had to change my memory_limit in php.ini, it worked as expected. I tried again on a new example project, and it is still working for me there as well.

@byrond
Copy link
Contributor

byrond commented Jan 31, 2022

When using this on an existing project, I encountered the following error:

     [exec]      Child process error (exit code 1): Drupal requires Prophecy PhpUnit when
     [exec]      using PHPUnit 9 or greater. Please use 'composer require --dev
     [exec]      phpspec/prophecy-phpunit:^2' to ensure that it is present.

Once I added the dependency, drupal-check worked as expected.

@byrond
Copy link
Contributor

byrond commented Feb 1, 2022

@becw I added phpspec/prophecy-phpunit:^2 as a dependency of the-build, since that message has come up a few times while using this branch.

@byrond
Copy link
Contributor

byrond commented Feb 25, 2022

The skeleton now installs version 3.0.4 of composer/xdebug-handler, which conflicts with drupal-check dependencies. I ran into this with a project using this branch, so I reran tests here. They failed, as expected. I will look for an update to drupal-check.

@byrond
Copy link
Contributor

byrond commented Feb 25, 2022

composer/composer requires either version 2 or 3 of composer/xdebug-handler, so version 3 gets installed during composer create-project. The composer require step needs to be able to downgrade that to version 2, since drupal-check doesn't support 3 yet. So, I added --with-all-dependencies to the composer require step in the CircleCI build job to allow it to happen.

@becw @eric-schmidt are we ready to merge this yet?

@becw becw merged commit e4b49e2 into develop Mar 11, 2022
@becw becw deleted the add-drupal-check-dependency branch March 11, 2022 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants