Add "extends" property to JSON format allowing for config file inheritance #1013

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

1stvamp commented Aug 19, 2012

Recently we've had the requirement to have multiple JSON files per deployment environment, e.g. testing.json and production.json configs for our CI and production environments that define different repositories to the development composer.json in order to use SSH aliases to get around the one-deploy-key-per-repo limitation for Github private repos.

Currently I'm using a shell script that is ran during CI and production deploys to generate a copy of the composer.json with hostnames replaced, in order to avoid duplicating lots of JSON properties.

The Python build tool buildout defines an extends property that allows you to inherit properties from another config file, e.g. have a main config file and then just override the properties you need to in your environment files.

I've added an extends property like this to the Composer JSON format that recursively adds parent properties during JsonFile::read().
Unittest too, 'natch.

This pull request passes (merged 4774585 into de6bb04).

@pborreli pborreli commented on an outdated diff Aug 19, 2012

src/Composer/Json/JsonFile.php
@@ -90,7 +90,15 @@ public function read()
throw new \RuntimeException('Could not read '.$this->path."\n\n".$e->getMessage());
}
- return static::parseJson($json, $this->path);
+ $parsed = static::parseJson($json, $this->path);
@pborreli

pborreli Aug 19, 2012

Contributor

incorrect indentation due to tabulation

Contributor

1stvamp commented Aug 19, 2012

@pborreli better?

This pull request passes (merged 2272745 into de6bb04).

Owner

Seldaek commented Aug 19, 2012

Refs #183 - I'm still not sure whether this feature is a good idea or not. If it's only for github keys, the workaround I use is to create a dummy user to whom I give readonly access to all the repos the CI needs. That user then has a single ssh key.

Contributor

1stvamp commented Aug 19, 2012

@Seldaek aye, in fact already using this, but extension has other use cases, e.g. overriding paths in production.

I can see the validity of the points raised in #183, but they're fairly easy to account for, for example see Rake, Buildout.

Merging for example is a simple "child overrides all defined properties of parent" in my patch, using array_replace.

Contributor

pborreli commented Aug 20, 2012

@1stvamp yes :)

@Seldaek Seldaek commented on the diff Oct 16, 2013

src/Composer/Json/JsonFile.php
@@ -90,7 +90,15 @@ public function read()
throw new \RuntimeException('Could not read '.$this->path."\n\n".$e->getMessage());
}
- return static::parseJson($json, $this->path);
+ $parsed = static::parseJson($json, $this->path);
+
+ if (!empty($parsed['extends']))
+ {
+ $parent = new JsonFile($parsed['extends']);
+ $parsed = array_replace($parent->read(), $parsed);
+ }
@Seldaek

Seldaek Oct 16, 2013

Owner

This code should be moved in Factory::createComposer because otherwise it's bound to create weird side effects at some point.

Now whether to use array_replace or not I'm not sure. I'd say it's probably enough for most things, but maybe for config or require it'd make sense to do an array_replace within the key? Maybe we should do it for all top level keys, so sort of recursive but not all the way, just array_replace per key if it's an array on both sides, and otherwise override it if it exists in the $parsed array here.

@1stvamp

1stvamp Oct 16, 2013

Contributor

All good points, I'll see if I can get it moved to Factory::createComposer and play around with replacing per key/if each is an array, sometime this week.

Cheers.

@stof stof commented on the diff Oct 17, 2013

doc/04-schema.md
+etc. which may need to override properties like `repositories`.
+
+Optional.
+
+### extends
+
+Defines a parent JSON configuration filepath to a JSON file that this configuration extends.
+
+All properties from the parent JSON file are loaded and any property defined in the current
+file override (replace) those properties. This is done recursively so if the parent JSON file
+defines a parent that will be loaded (with any properties of it are also replaced) and so on.
+
+Useful for creating environment specific configurations, e.g. `testing.json`, `production.json`
+etc. which may need to override properties like `repositories`.
+
+Optional.
@stof

stof Oct 17, 2013

Contributor

the doc is duplicated

@1stvamp

1stvamp Oct 17, 2013

Contributor

Probably due to a bad rebase, will remove dupe.

So where exactly is this up to in terms of being merged? I have a very similar use-case for this to what @gggeek mentioned in issue #183 and would like to be able to have this functionality for something I'm developing. It seems strange that this has been going on for two years and hasn't been fully dealt with.

Contributor

unn commented Oct 22, 2014

+1

Desperately waiting for this one. Please solve your merge conflicts and submit it again,

Contributor

1stvamp commented Jul 20, 2015

This PR has been superseded by #4210 (in which @aripringle has refactored everything so it'll merge and work with latest composer HEAD).

1stvamp closed this Jul 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment