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.

Sign up for GitHub

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

[DRAFT] Isolating bin dependencies. #1716

Closed
wants to merge 4 commits into from
Closed

[DRAFT] Isolating bin dependencies. #1716

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce3595e
Isolating bin dependencies.
oleg-andreyev Oct 24, 2023
db702e3
normalized
oleg-andreyev Oct 24, 2023
ef5fdb8
returned phpunit into main composer.json
oleg-andreyev Oct 25, 2023
e9841ff
reverted bootstrap.php
oleg-andreyev Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ phpcs.xml.dist export-ignore
psalm.xml.dist export-ignore
/Resources/doc export-ignore
/Tests export-ignore
/vendor-bin/**/composer.lock binary
5 changes: 5 additions & 0 deletions .github/workflows/continuous-integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ jobs:
ini-values: "zend.assertions=1"
extensions: "pdo_sqlite"

- name: Install dependencies
run: |
composer update
Copy link
Member

Choose a reason for hiding this comment

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

this should not be done here.

Instead, you need to move the command running composer bin * later in the workflow, after the step Install dependencies with Composer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Composer usually complains that composer.lock is missing and performs update internally

Copy link
Member

Choose a reason for hiding this comment

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

and that's the whole issue: you perform the install or update before the steps that modify the composer.json to change what will get installed.

composer bin all install
Copy link
Member

Choose a reason for hiding this comment

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

why installing all ? The psalm bin needs to be installed only for the psalm job.


- name: "Globally install symfony/flex"
run: "composer require --no-progress --no-scripts --no-plugins symfony/flex"

Expand Down
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ phpunit.xml
/.phpunit.result.cache
/phpcs.xml
/.phpcs-cache
/vendor-bin/**/vendor/
!/vendor-bin/**/composer.lock
2 changes: 1 addition & 1 deletion Tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

use Doctrine\Deprecations\Deprecation;

require_once 'vendor/autoload.php';
require_once __DIR__ . '/../vendor-bin/phpunit/autoload.php';

Deprecation::enableWithTriggerError();
19 changes: 12 additions & 7 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,13 @@
"symfony/service-contracts": "^1.1.1 || ^2.0 || ^3"
},
"require-dev": {
"bamarni/composer-bin-plugin": "^1.5",
"doctrine/annotations": "^1 || ^2",
"doctrine/coding-standard": "^9.0",
"doctrine/deprecations": "^1.0",
"doctrine/orm": "^2.14 || ^3.0",
"friendsofphp/proxy-manager-lts": "^1.0",
"phpunit/phpunit": "^9.5.26 || ^10.0",
"psalm/plugin-phpunit": "^0.18.4",
"psalm/plugin-symfony": "^4",
"psr/log": "^1.1.4 || ^2.0 || ^3.0",
"symfony/phpunit-bridge": "^6.1",
"symfony/property-info": "^5.4 || ^6.0",
"symfony/proxy-manager-bridge": "^5.4 || ^6.0",
"symfony/security-bundle": "^5.4 || ^6.0",
Expand All @@ -63,8 +60,7 @@
"symfony/var-exporter": "^5.4 || ^6.2 || ^7.0",
"symfony/web-profiler-bundle": "^5.4 || ^6.0",
"symfony/yaml": "^5.4 || ^6.0",
"twig/twig": "^1.34 || ^2.12 || ^3.0",
"vimeo/psalm": "^4.30"
"twig/twig": "^1.34 || ^2.12 || ^3.0"
},
"conflict": {
"doctrine/annotations": ">=3.0",
Expand All @@ -89,16 +85,25 @@
},
"config": {
"allow-plugins": {
"bamarni/composer-bin-plugin": true,
"composer/package-versions-deprecated": true,
"dealerdirect/phpcodesniffer-composer-installer": true,
"symfony/flex": true
},
"extra": {
"bamarni-bin": {
"bin-links": true,
"forward-command": true,
"target-directory": "vendor-bin"
}
},
"sort-packages": true
},
"scripts": {
"auto-scripts": {
"cache:clear": "symfony-cmd",
"assets:install %PUBLIC_DIR%": "symfony-cmd"
}
},
"bin": "echo 'bin not installed'"
}
}
8 changes: 5 additions & 3 deletions psalm.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://getpsalm.org/schema/config"
xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
autoloader="vendor-bin/psalm/autoload.php"
>
<plugins>
<pluginClass class="Psalm\SymfonyPsalmPlugin\Plugin"/>
<pluginClass class="Psalm\PhpUnitPlugin\Plugin"/>
</plugins>

<pluginClass class="Psalm\PhpUnitPlugin\Plugin"/></plugins>
<projectFiles>
<ignoreFiles>
<directory name="vendor"/>
<directory name="vendor-bin/"/>
<!-- Deprecated classes, not worth fixing -->
<file name="Command/ImportMappingDoctrineCommand.php"/>
<file name="Command/Proxy/OrmProxyCommand.php"/>
Expand Down Expand Up @@ -58,7 +60,7 @@
<UndefinedDocblockClass>
<errorLevel type="suppress">
<!-- https://github.com/symfony/symfony/issues/45609 -->
<referencedClass name="UnitEnum" />
<referencedClass name="UnitEnum"/>
<directory name="DependencyInjection"/>
<directory name="Tests/DependencyInjection"/>
</errorLevel>
Expand Down
4 changes: 4 additions & 0 deletions vendor-bin/phpunit/autoload.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?php

require_once __DIR__ . '/vendor/autoload.php';
require_once __DIR__ . '/../../vendor/autoload.php';

Check warning on line 4 in vendor-bin/phpunit/autoload.php

View check run for this annotation

Codecov / codecov/patch

vendor-bin/phpunit/autoload.php#L3-L4 

Added lines #L3 - L4 were not covered by tests
6 changes: 6 additions & 0 deletions vendor-bin/phpunit/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"require-dev": {
"phpunit/phpunit": "^9.5.26",
Copy link
Member

Choose a reason for hiding this comment

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

PHPUnit runs in the same process than our code so it cannot have different dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will work fine, external tools and their dependencies should not affect application/lib dependencies. Had many occasions that phpunit dependency example symfony/process conflicts with app/lib dependencies and had to upgrade phpunit first.

Copy link
Member

Choose a reason for hiding this comment

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

That's the reason they must be compatible: when running tests, PHPUnit will run the tested code inside its own process. And as PHP cannot load 2 definitions of a class, your code will run against the Process class of the PHPUnit dependency instead of using its own dependency, which will break.

Isolating tools is possible when the tool does not execute your own code in its process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right , phpunit is probably an exception , other tools like psalm could be moved as they don't autoload code

Copy link
Member

Choose a reason for hiding this comment

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

And that's why I reported the issue for PHPUnit, not for Psalm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted phpunit.

"symfony/phpunit-bridge": "^6.1"
}
}
Loading
Loading