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 ability to merge remote composer configurations via http #98

Closed
wants to merge 1 commit into from

Conversation

codeandcountry
Copy link

I needed to pull a centralized list of required packages into multiple composer configs. Since composer has the ability to read JSON contents from a remote source via HTTP it was a straight forward change to composer-merge-plugin to correctly validate that the url exists (but still use glob for local files) and pass a RemoteFileSystem object to JsonFile constructor if we're dealing with a URL.

@codeandcountry
Copy link
Author

One test fails, but it fails on your master branch too so I didn't mess with it.

@dereckson
Copy link
Member

Thanks for the contribution.

Could you squash these two commits (I've noticed the second amend a code introduced by the first)?

@codeandcountry
Copy link
Author

@dereckson - yessir, commit updated

@codeandcountry
Copy link
Author

@dereckson - thanks for taking the time to look at this so quickly! I have refactored the section you commented on, switched to using preg_match to search for 200,301,302,307 and 308 as those are the only response codes we want to accept.

@@ -107,7 +115,12 @@ public function getRequires()
*/
protected function readPackageJson($path)
{
$file = new JsonFile($path);
if(substr($path, 0, 4) === 'http'){
Copy link
Member

Choose a reason for hiding this comment

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

This probably should use a preg_match for ^https?:// instead of just grabbing anything starting with "http". As written an import of "http_module/composer.json" would get slurped up to be treated as a remote file.

@bd808
Copy link
Member

bd808 commented Jan 1, 2016

This needs documentation updates as well.

@bd808 bd808 added this to the 1.4.0 milestone Jan 1, 2016
@bd808 bd808 self-assigned this Jan 1, 2016
@joshuataylor
Copy link

I would love to see this happen, what do I need to do to make it so?

@joshuataylor
Copy link

Here is the updated patch, not sure if I should create another PR. Please credit this to the OP, as this was only 5 minutes of work.

https://gist.github.com/joshuataylor/b48deb6cc76c053164fba6ac6dce31f1

@JanZerebecki
Copy link

Does this obey the secure-http setting of composer?

@mikkeschiren
Copy link

The patch seems to change coding not relevant to the change.
Den 21 apr 2016 15:17 skrev "Jan Zerebecki" notifications@github.com:

Does this obey the secure-http setting of composer?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#98 (comment)

@bd808
Copy link
Member

bd808 commented Apr 21, 2016

@joshuataylor Just glanced quickly at https://gist.github.com/joshuataylor/b48deb6cc76c053164fba6ac6dce31f1 and it has some problems with respect to php version support. This plugin supports PHP >=5.3.2 so it can't use short array syntax. I didn't look much deeper than that to see if you had actually fixed up all the things I had commented on for the current patch from @codeandcountry.

If you really want to take over this patch then I would suggest that you pull the PR into a branch in your fork, fix things up with additional patches, and submit a new PR. Sadly github doesn't have much better support than that for collaboratively working on a PR unless @codeandcountry grants you push rights to his fork.

Generally speaking I'd class the idea behind this PR as "spooky". It certainly needs to be careful not to introduce a regression of https verification in newer versions of Composer which support cert chain verification and enforcement. I honestly don't understand how including a shared config via https is different than including a published package which is a well supported Composer use case (actually it's primary use case). I'd like to understand the motivation more deeply before adding support for this even if the patch is in good shape.

@bd808 bd808 removed this from the 1.4.0 milestone Jul 11, 2016
@bd808 bd808 removed their assignment Jul 11, 2016
@buric
Copy link

buric commented Jan 9, 2017

Good idea @codeandcountry. I made a workaround with scripts to download remote files.

    "extra": {
        "merge-plugin": {
                "require": [
                        "packages1.json",
                        "packages2.json"
                ]
        }
    },
    "scripts": {
        "pre-dependencies-solving": [
                "wget -q -O packages1.json http://example.com/packages1.json",
                "wget -q -O packages2.json http://example.com/packages2.json"
        ]
    }

phayes added a commit to phayes/composer-merge-plugin that referenced this pull request May 19, 2017
@bd808
Copy link
Member

bd808 commented Oct 27, 2020

Closed for inactivity.

@bd808 bd808 closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants