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

Slip stream in composer 2.0 #18

Merged
merged 20 commits into from
Jul 1, 2020

Conversation

boesing
Copy link
Member

@boesing boesing commented May 23, 2020

Q A
New Feature yes

Description

While trying to migrate to composer 2.0, I've tried many ways on how to achieve the functionality of the current plugin.

The current plugin "slip streams" into the composer process, which is probably the best solution to solve the following scenarios:

  • If 3rd-party library has a zend-dependency and updates to laminas, there wont be any changes to the project as the laminas package is already required.

  • If a 3rd-party library has a zend-dependency and requires a newer zend-dependency, the project will just upgrade to the laminas equivalent. No further action needed.

  • If a 3rd-party library totally drops a zend-dependency and does not need the laminas equivalent, the slip-streamed laminas dependency will automatically being removed from the project if its not needed anymore.

I came up with a way in #15 which should have introduced some more user interactions. The main idea was:

If a 3rd-party library has a zend-dependency, ask the user if he wants to replace that zend-dependency the laminas pendant, write the dependency to composer.json of the project and remember that we introduced that dependency due to a non-migrated library.

TL;DR

This is finally ready for review.

Story on why I decided to go this way

After many sleepless nights and a total rewrite of the dependency plugin, I realized that this approach would probably be a future proof solution but its way too complex.
The complexity exceeded my brain capacity too often.
Its unmaintainable due to all those possible combinations:

  • 3rd-party library no.1 uses zend-dependency and is required via root composer.json as a require package
  • 3rd party library no.2 uses same zend-dependency and is required via root composer.json as a require-dev package
  • 3rd party library no.1 is being uninstalled
  • root composer.json should change the require of the laminas equivalent to require-dev as there is only a dev-requirement left which requires the zend-package

And thats just one of those complex scenarios...

However, after I realized that #15 wont be a solution I am happy with, I started to work on this approach today. I don't know why I tried this earlier (as I was debugging what exactly changed in composer 2.0 that the slip-stream solution we are actually use does not work anymore)... That would probably saved my a ton of work.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing changed the base branch from master to develop May 23, 2020 21:30
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing marked this pull request as ready for review May 23, 2020 21:47
@boesing boesing added this to In progress in Composer v2 via automation May 23, 2020
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing mentioned this pull request May 23, 2020
9 tasks
@boesing boesing linked an issue May 24, 2020 that may be closed by this pull request
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member Author

boesing commented May 25, 2020

Well, now travis fails because codestandard says "closure can be static" but it can't as it wont set the property anymore... Thats why I hate changing code I didn't wrote myself... 👎

@samsonasik
Copy link
Member

I didn't write "static" in my example #18 (comment) , you added static yourself, that's why it got error.

@boesing
Copy link
Member Author

boesing commented May 25, 2020

I didn't write "static" in my example #18 (comment) , you added static yourself, that's why it got error.

I've applied phpcbf which changed it to static without me noticing.
After tests failed, I dropped static and now phpcs is arguing about missing static.

Welcome to QA hell.

Closure binding only works if the function is non-static

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member Author

boesing commented May 25, 2020

@weierophinney Any ideas how to raise code coverage? it needs to get merged from both results of COMPOSER_VERSION v1 and v2...

@weierophinney
Copy link
Member

After tests failed, I dropped static and now phpcs is arguing about missing static.

Welcome to QA hell.

Wrap the closure (or at least its opening line) with // phpcs:disable and // phpcs:enable, if tests fail when it is marked as static.

@weierophinney
Copy link
Member

Any ideas how to raise code coverage? it needs to get merged from both results of COMPOSER_VERSION v1 and v2...

No clues whatsoever... maybe @michalbundyra might have ideas?

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
composer.json Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@781d6f7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop      #18   +/-   ##
==========================================
  Coverage           ?   92.33%           
  Complexity         ?       97           
==========================================
  Files              ?        5           
  Lines              ?      339           
  Branches           ?        0           
==========================================
  Hits               ?      313           
  Misses             ?       26           
  Partials           ?        0           
Flag Coverage Δ Complexity Δ
#composer1 52.80% <0.00%> (?) 97.00% <0.00%> (?%)
#composer2 69.32% <0.00%> (?) 97.00% <0.00%> (?%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 781d6f7...2e12ce2. Read the comment docs.

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member Author

boesing commented Jun 2, 2020

Well, I finally got proper codecoverage.
Its around 92% now.

Hopefully, thats enough. I also removed dev-dependency of composer/semver and thus, I think we're ready for a release candidate.

Composer v2 automation moved this from In progress to Reviewer approved Jun 3, 2020
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

This looks fantastic - surprised to see you found a way to keep the current functionality with the v2 API - I'm assuming the "POOL" events must be the secret to making that work?

I have no feedback at a technical level. I haven't had time to dive into where and when the POOL events are triggered, but I trust you did the research!

Let's do an alpha or beta release from this, and have people start testing!

@@ -1,14 +1,18 @@
<?xml version="1.0"?>
<ruleset name="Webimpress Coding Standard">
<rule ref="./vendor/webimpress/coding-standard/ruleset.xml"/>
<rule ref="./vendor/webimpress/coding-standard/ruleset.xml">
Copy link
Member

Choose a reason for hiding this comment

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

We should probably move to Laminas Coding Standard v2, but that can wait for another PR. 😄

@boesing
Copy link
Member Author

boesing commented Jun 10, 2020

I'm assuming the "POOL" events must be the secret to making that work?

Yah, more or less. Jordi pointed me to this event, but it is being triggered before any package is being downloaded. As I've started to implement the initial version, I realized, that the dependency-plugin was not able to interact due to the fact, that it was not downloaded at the point when the events are triggered (in a project which is being installed for the first time - so without the vendor/ directory available).

That was what made me thinking about the way we discussed a few months ago. That other way drove me crazy after a few weeks of developing and thus I tried this approach again.
However, I am happy that this works, even if I dont like the fact that we have to re-run composer again to ensure we have our packages slip-streamed.

Flow is as follows (for a freshly installed package):

  • Pool event is being triggered
  • Packages are being downloaded to vendor/
  • Packages are being triggered (install or update event)
  • Plugin detects zend packages
  • Composer executes dump-autoloader
  • Plugin hooks into that and executes uninstall for all zend packages
  • Plugin executes composer update --lock
  • Pool event is being triggered
  • Plugin changes zend packages to laminas packages in the pool
  • composer.lock is being written with the laminas replacements
  • Composer installs laminas package in favor of zend package
  • dump-autoload is being triggered again (this time we dont care as we did our job)

So hopefully, we have the same features as in the first version of this plugin.

@weierophinney I would love to see this as a first preview release so contributors can start testing with the new version.

@weierophinney
Copy link
Member

@boesing Go ahead and merge to develop, and issue a 2.0.0beta1 release. I'm suggesting beta, in case we determine there might be additional features before we do a stable release; that way we can do another beta. Otherwise, we can jump directly from beta to stable if user tests show it working as expected.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
weierophinney added a commit that referenced this pull request Jul 1, 2020
- Removes empty 1.0.5 stub from CHANGELOG
- Renames 1.1.0 release to 2.0.0beta1 in CHANGELOG
- Adds CHANGELOG entry for #18
- Updates dev branch alias to 2.0.x-dev

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney merged commit 2e12ce2 into laminas:develop Jul 1, 2020
Composer v2 automation moved this from Reviewer approved to Done Jul 1, 2020
@boesing boesing deleted the feature/slip-stream-2.0 branch July 1, 2020 21:41
@roelvanduijnhoven
Copy link

Just want to give feedback: works great for me with the composer 2.0 plugin :). Just pushed our app to production, using this beta.

},
"require-dev": {
"composer/composer": "^1.9",
"dealerdirect/phpcodesniffer-composer-installer": "^0.5.0",
"composer/composer": "^1.9 || ~2.0.0@dev || ^2.0",
Copy link
Member

Choose a reason for hiding this comment

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

~2.0.0@dev can be removed after stable 2.0


abstract class AbstractDependencyRewriter implements RewriterInterface
{
/** @var Composer */
Copy link
Member

Choose a reason for hiding this comment

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

Can be null

/** @var Composer */
protected $composer;

/** @var IOInterface */
Copy link
Member

Choose a reason for hiding this comment

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

Can be null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Composer v2
  
Done
Development

Successfully merging this pull request may close these issues.

Adds support for Composer v2
7 participants