Problem/Motivation

If the "bin-dir" setting is changed in "composer.json", then testing fails because "vendor/bin/" is hardcoded in a few places key to running tests.

This was being respected up until 11.3.x, probably because it was using the old `'core/scripts/drupal'` script, instead of the new `dr` one.

Steps to reproduce

- https://git.drupalcode.org/project/drupal/-/blob/main/core/tests/Drupal/...
- https://git.drupalcode.org/project/drupal/-/blob/main/core/tests/Drupal/...

We can see an example of running PHPUnit tests in here https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/10...

Composer setting: https://git.drupalcode.org/project/gitlab_templates_downstream/-/blob/d1...

    "config": {
        "bin-dir": "custom-bin-folder/is/here"
    }

Proposed resolution

Read the setting instead of hardcoding it.

Remaining tasks

MR

CommentFileSizeAuthor
#14 3609757-nr-bot_uoqp2hcq.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3609757

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fjgarlin created an issue. See original summary.

fjgarlin’s picture

fjgarlin commented

Adding related issue.

fjgarlin’s picture

fjgarlin commented

The same happens if "vendor-dir" is customized.

fjgarlin opened merge request !16277

fjgarlin’s picture

fjgarlin commented
Status: Active » Needs review

Ready for review.

* I used claude to help with the solution.

jonathan1055’s picture

jonathan1055 commented

This looks like a good solution, very comprehensive, covering all types of customized value. The test coverage looks good too.

I am surprised that core testing got this far with all the hard-coded instances you have found. Maybe not many sites have a custom bin-dir in real life?

fjgarlin’s picture

fjgarlin commented

This was tested in https://git.drupalcode.org/project/gitlab_templates/-/work_items/3572419... as well.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Is this useful outside of tests? Wondering if this is something we want to maintain in Utility, or whether it should just be a trait that can be included in tests that need it.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

We have core/tests/Drupal/Tests/Composer/ComposerIntegrationTrait.php which currently contains a single static method to find composer.json files in core, a bin dir finder could fit in there I think?

I can't immediately think of a use case for this at runtime, so this will be easier to maintain in a test trait than it will be if we add it to the Utility component.

fjgarlin’s picture

fjgarlin commented
Status: Needs work » Needs review

I moved the method to the suggested file and changed the references to it. Needs review again.

jonathan1055’s picture

jonathan1055 commented

Here is an example of the problem, in the d11-recipe branch of Gitlab Templates Downstream. Both the Drupal CMS and the current and future PHPUnit jobs fail with Could not open input file: vendor/bin/dr
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

The tests use this change and I have re-run the pipeline following the work done in #10. The PHPUnit jobs still pass with the MR.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Next, I'm trying to patch the Drupal CMS job.

jonathan1055’s picture

jonathan1055 commented

With the Drupal CMS job patched too, that job ends green.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

So this indicates that MR16277 is doing the right thing for core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php which is used on both of the contrib-facing tests.

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Reviewed & tested by the community

The feedback from @longwave in #9 has been addressed, as the method is now moved into ComposerIntegrationTrait. The changes look good, the internal test coverage shows that the various outputs are all covered, and my testing in a real recipe project shows that with this MR both the Drupal CMS test and the regular phpunit testing are fixed.

So it looks like we can move it along to RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3609757-nr-bot_uoqp2hcq.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

fjgarlin changed the visibility of the branch main to hidden.

fjgarlin changed the visibility of the branch 3609757-composer-bin-dynamic to hidden.

fjgarlin’s picture

fjgarlin commented
Status: Needs work » Reviewed & tested by the community

Today's commit https://git.drupalcode.org/project/drupal/-/commit/854ccc02f134c81abf0b2... moved some logic into a different file. I rebased and applied exactly the same changes in the expected place.

No change in logic and code (other than the described), test results are exactly the same, so setting back to RTBC.

fjgarlin’s picture

fjgarlin commented

Note: i also move the test to a folder where there is a similar test. The current location of the test file made it be the only test in that folder, which was full of traits. I'm happy to revert the last commit, but I thought it made sense.

  • longwave committed 33724975 on 11.4.x 
    test: #3609757 Composer bin-dir setting is ignored

By: fjgarlin
By:...

  • longwave committed b265e035 on 11.x 
    test: #3609757 Composer bin-dir setting is ignored

By: fjgarlin
By:...

  • longwave committed a8ea302a on main 
    test: #3609757 Composer bin-dir setting is ignored

By: fjgarlin
By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Needed some minor fixup after the directory move but I think that's ok. Also didn't backport cleanly because of the baseline diff, but I think I fixed that in the cherry-pick instead of sending it back for a new MR.

Committed a8ea302 and pushed to main. Thanks!

Committed b265e03 and pushed to 11.x. Thanks!

Committed 3372497 and pushed to 11.4.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !16277

jonathan1055’s picture

jonathan1055 commented

Thanks for merging, and for the backport to 11.4.x
I have just confirmed in the Gitlab Templates test branch that the "next" variants, using 11.4.x and 11.x, are fixed.