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

FIX #6950 - Add wikimedia/composer-merge-plugin #6976

Merged

Conversation

Abuelodelanada
Copy link
Contributor

Description

Fix #6950

Motivation and Context

Fix #6950

How To Test This

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@Dillon-Brown Dillon-Brown added the PR:Community Contribution These are contribution made by the community label Feb 20, 2019
@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (hotfix-7.10.x@61358aa). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             hotfix-7.10.x    #6976   +/-   ##
================================================
  Coverage                 ?    7.47%           
================================================
  Files                    ?     3736           
  Lines                    ?   386774           
  Branches                 ?        0           
================================================
  Hits                     ?    28917           
  Misses                   ?   357857           
  Partials                 ?        0

@Dillon-Brown Dillon-Brown added the PR:Type:Enhancement Pull requests that provide more functionality. Associated Issues are called suggestions label Mar 21, 2019
@PedroErnst
Copy link
Contributor

Looks like a nice addition. Should composer.ext.json maybe reside in the custom folder?

@Jason-Dang
Copy link
Contributor

Hi, I think this would be a really cool and useful feature for upgrade safe composer packages. Though I seen the include file is composer.ext.json. Would it be possible to add this to the extensions framework, so that files can be added to custom/Extension/application/Ext/Composer/ and on QR+R get compiled down to custom/application/Ext/Composer/composer.ext.json?.

@PedroErnst
Copy link
Contributor

@Jason-Dang

Really good point.
It would appear that updating the following line in composer.json will take care of compiling all the files from the Extension framework (and any subdirectory in the Composer folder):

      "merge-plugin": {
          "include": [
              "composer.ext.json",
              "custom/Extension/application/Ext/Composer/*/*.json"
          ],

Maybe it would be a good idea to include this in the PR?

@Jason-Dang
Copy link
Contributor

@PedroErnst That sounds like the best solution to me as it eliminates the need for a lot of extra work building the extension file etc.

Last thing I think needs to be considered is version conflicts for the same package. Assuming composer uses the highest version, what if something in the application requires an older version?

This also appears to be a duplicate of #6975

@Abuelodelanada
Copy link
Contributor Author

@Jason-Dang

Really good point.
It would appear that updating the following line in composer.json will take care of compiling all the files from the Extension framework (and any subdirectory in the Composer folder):

      "merge-plugin": {
          "include": [
              "composer.ext.json",
              "custom/Extension/application/Ext/Composer/*/*.json"
          ],

Maybe it would be a good idea to include this in the PR?

Hi @PedroErnst @Jason-Dang

Do you think I should modify the PR with this?

I was looking for something like the config_override.php but for composer plugins... that's why I only set one file called: composer.ext.json in root directory.

@PedroErnst
Copy link
Contributor

@Abuelodelanada

Yes I think as Jason pointed out this would keep in line with the Extension framework... different customizations could add composer requires without needing to be aware of eachother.

I've tested and if the folder in question is missing or empty it doesn't throw an error. So if someone wants to go with the simple solution you propose with the composer.ext.json that works fine, but it also leaves the door open for more complex setups down the line. Best of both worlds.

@Abuelodelanada
Copy link
Contributor Author

@PedroErnst

Great! So, do you need I made a fix to this PR?

@PedroErnst
Copy link
Contributor

Ultimately it's up to you, if you think it makes sense what we suggest then you're welcome to include it in your PR. I would say yes, but I'm just another dev :)

@Abuelodelanada
Copy link
Contributor Author

Ultimately it's up to you, if you think it makes sense what we suggest then you're welcome to include it in your PR. I would say yes, but I'm just another dev :)

Done! Thanks @PedroErnst and @Jason-Dang for the suggestion!

@Jason-Dang
Copy link
Contributor

Assessed & Approved.

@Dillon-Brown Dillon-Brown added the Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member label Apr 19, 2019
@Dillon-Brown Dillon-Brown merged commit 6d85400 into salesagility:hotfix-7.10.x Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR:Community Contribution These are contribution made by the community PR:Type:Enhancement Pull requests that provide more functionality. Associated Issues are called suggestions Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants