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

using composer.json with project #360

Closed
Remo opened this issue Jul 3, 2014 · 29 comments
Closed

using composer.json with project #360

Remo opened this issue Jul 3, 2014 · 29 comments

Comments

@Remo
Copy link
Contributor

Remo commented Jul 3, 2014

I'm a bit baffled about the composer.json which isn't in the web directory, but rather in web/concrete.

What's the idea behind this? Should we add a custom composer.json in the top-level directory if we want to use some libraries from packagist in our own project? We'd end up with two autoloaders which feels odd too and if I want to use composer.json in a package, it would get even worse.

Why not keep the vendor directory as a top-level directory? I'm no composer expert, but isn't this the way it was intended to be? What am I missing?

@mkly
Copy link
Contributor

mkly commented Jul 3, 2014

👍

@Remo
Copy link
Contributor Author

Remo commented Jul 17, 2014

I've been playing around with a few things, the longer I play with 5.7, the more I think this should be changed. I've added a package specific library with a composer.json in my own package. The result of this is having two vendor directories and since my library uses a library already included in the concrete5 core, I have more files than necessary and some odd effects.

Having just written that, if we simply move composer.json a level higher, we wouldn't have the perfect solution. It would mean that if I wanted to add a library from packagist I'd have to modify the original composer.json from concrete5 which if it gets changed in the future would mean that I have to manually merge it.

Ideally, we'd be able to use this command to get things started: composer create-project concrete5/concrete5 my-project-name --prefer-dist Not sure if that's easy with the structure we already have, but imho that's the way it should be.

@fabian
Copy link
Contributor

fabian commented Sep 2, 2014

I was also a bit confused when I first saw composer.json not being on the top level - but I actually think now not the position of the composer.json should be changed, but the repository should be split into concrete5 and concrete5-core. The concrete5 repository would then have a dependency on concrete5-core which could be installed to the appropriate location with a custom Composer installer.

Another alternative could be to publish the core as a Git subtree. Symfony does something similar by having everything in their main repository and then publishing individual components as subtree splits. GitHub has also a help article for how to do this.

@Remo
Copy link
Contributor Author

Remo commented Sep 2, 2014

that's okay for me too! All I'm asking for is a nice way to add more project specific composer dependencies.

@aembler
Copy link
Member

aembler commented Sep 2, 2014

@fabian I'm not sure that works well with the way we tell people to approach overriding core components of concrete5.

@Remo What about updates? We're delivering fully updated third party libraries in the concrete5 build so when 5.7.1 comes out and requires updates, and I point to a new core, how do I update the libraries that are powered by composer and live in the root vendor directory. Teach people to use composer? That doesn't sound very fun. I'm not sure you can get around multiple vendor directories, or why that's intrinsically a bad thing. If my S3 plugin requires Aws from composer, I can just bundle that with my S3 package and everything just works. If we wanted to use our own composer libraries in the application directory, what about a composer.json that you could include in your application directory, and then checking to see if application/vendor/autoload.php exists before inclusion in dispatcher?

I agree that having one vendor directory in the root makes some sense in the abstract, but unless we can figure out how that would work with updates I don't think it can be the solution.

@mkly
Copy link
Contributor

mkly commented Sep 2, 2014

@andrew I think the issue comes up when concrete5 is not your primary application. For example if I were to be building a side app for a site that wanted to do chat so I need sockets and an event loop like react https://github.com/reactphp

I don't want to speak for @Remo and @fabian who likely have a better understanding of the use cases but this may be one of those monolithic vs component discussions.

@Remo
Copy link
Contributor Author

Remo commented Sep 2, 2014

@aembler think of concrete5 more as a framework. You still have multiple composer.json files, but you keep the libraries in one place. I'd basically create my project specific composer.json like this:

require: {
   "concrete5/concrete5": "~5.7",
   "remo/library": "dev-master"
}

You can freely update your own libraries, but we won't have any conflicts if a library I'm using in my project is used by concrete5 too. Basically the way laravel, fuelphp etc. work.

@KorvinSzanto
Copy link
Member

I think I'm just reiterating what @fabian was articulating. I say we provide the top level composer.json and maintain separated concrete5/concrete5 (the top level project structure) and concrete5/core (the concrete directory) and rework things to expect the core to be at '/vendors/concrete5' rather than '/concrete'.

With this setup, we can provide the same experience to an end user that may update through our updater through pre-building a zip, and provide a more powerful, more standard structure for someone that may know how to use it through the zip file or the github repo.

To reiterate with more simplicity:

/web/concrete -> github.com/concrete5/concrete5-core

/web/composer.json -> { require: { "concrete5/concrete-core" : '5.7.*' } }

@aembler
Copy link
Member

aembler commented Sep 4, 2014

How would we handle our in-dashboard updates under that system?

@Remo
Copy link
Contributor Author

Remo commented Sep 4, 2014

I'd remove the composer files in the ZIP release and keep them in github/packagist only

@aembler
Copy link
Member

aembler commented Sep 4, 2014

I guess it's more about changes in vendor/ libraries from one version to the next.

So, 5.7.0 ships and it looks like this

web/application
web/packages
web/updates/ (empty)
web/vendor/concrete5-core/src/etc...
web/vendor/composer/
web/vendor/punic/
web/vendor/monolog
etc...

So then, as we do today, you download 5.7.1.

Now you have.

web/application/
web/updates/concrete5.7.1

All your vendor libraries in the root are still 5.7.0. I imagine the 5.7.1 that's in updates/ is the 5.7.1 pulled from the concrete5-core/ repository, which make sense – but now you have to update your composer autoload file somewhere.

This can perhaps be mitigated if there's a way to auto update composer from within the browser – and obviously you'd have to be able to write to the directories or make them writable. But one of the nice things about concrete5 now is the ability to download multiple updates and have them setting in the updates directory and apply one when you're ready.

Thoughts? I'm also not super crazy about having to teach people that, in order to override the core, instead of copying from concrete/blocks/page_list/view.php to application/blocks/page_list/view.php, they have to copy from vendor/concrete5-core/blocks/page_list/view.php into the application directory. Small change – but it's much less discoverable this way. That's not a dealbreaker though.

@KorvinSzanto
Copy link
Member

@aembler Yeah, you'd just have to overwrite the classmap. In the config change, I wrote an array to declaration string class, so it's be pretty easy to do.

@Remo
Copy link
Contributor Author

Remo commented Sep 5, 2014

@aembler didn't get what you mean first, but I see the problem.

To summarize things: With the current structure - assuming that c5 needs punic and my own package too. We'd have a structure like this:

/concrete/vendor/punic
/app/packages/remo_pkg/vendor/punic

I might not even need punic directly, but it might get installed in my package because of another library I'm using. Hard to avoid and if we even use different versions, we'd end up with some funny conflicts.

If we share a single vendor directory (like I suggested), we'd have a hard time to upgrade concrete5. Rewriting the class loader would be the easiest part, but you'd basically have to make sure that if you move to a new vendor directory, the libraries not used by c5 are copied too.
The other approach would be to keep the vendor directory where it is and overwrite a number of libraries during the upgrade process, but that's super ugly too.

Honestly, I'm not sure there's a nice way to use composer while also being able to upgrade concrete5 in the dashboard. A while ago, I tried to run composer from a SAPI instead of CLI but I ran into some issues because it tries to call proc_open and other methods I wouldn't rely on. It also feels like a bad idea to run "composer install" from a web interface, even though it would manage all the versioning stuff quite nicely.

@fabian
Copy link
Contributor

fabian commented Sep 9, 2014

I'm wondering if the upgrade process in the dashboard is really needed by someone who's using Composer? If I would want to upgrade concrete5 in my Composer setup it makes more sense for me to change "concrete5/core": "5.7.1" to "concrete5/core": "5.7.2" than to go into the dashboard and letting concrete5 override files downloaded by Composer. The update of all dependencies would then also be handled nicely by Composer.

Having a separate Core and using only one vendor directory in the root has another advantage: I can use newer versions of the dependencies required by the Core. For example I might want to use symfony/serializer 2.6 instead of 2.5 because of some bug fixes or new features.

There's also no need to rewrite the Core to run from vendor/concrete5/core (as described by @KorvinSzanto), we can use a Composer installer type to copy the Core to web/concrete.

So people using the source version of concrete5 directly from GitHub would be required to use Composer (they are already), have all dependencies in the vendor folder in the root directory and it's recommended they avoid the dashboard upgrade process. The official build downloaded from the concrete5 website can still be delivered with all libraries bundled inside concrete/vendor as before.

@aembler You raised some concerns about overriding Core components which I didn't really understand… was that because of potential naming confusions?

@Remo
Copy link
Contributor Author

Remo commented Sep 9, 2014

As a regular composer user, I certainly don't need (and don't want) the dashboard upgrade functionality. As long as both ways are possible this should be fine!

I think Andrew meant that if you want to customize a view.php of a block, you'd have to dig around the vendor directory to find the original copy of the file. People used to look in /concrete might be a bit confused. I don't think this is a big problem as the structure is quite different anyway.

@fabian
Copy link
Contributor

fabian commented Sep 9, 2014

Well again, if we copy the Core into /concrete where it used to be (with a Composer installer), nothing changes there.

@Remo
Copy link
Contributor Author

Remo commented Sep 9, 2014

Yes

@aembler aembler modified the milestones: 5.7.2, 5.7.1 Oct 6, 2014
@aembler
Copy link
Member

aembler commented Oct 6, 2014

There hasn't been any movement on this so it's bumping to 5.7.2.

@fabian
Copy link
Contributor

fabian commented Oct 6, 2014

How would you like us to proceed? Unfortunately I can't put the changes into a PR as they need the creation of a new GitHub repository as documented above.

@aembler
Copy link
Member

aembler commented Oct 6, 2014

We're a bit swamped with a release this week, and this isn't really in the
forefront of our minds. We don't want to hold it up but we already have a
problem with our main, active development platform (5.7) not being a part
of our main, canonical repo (concrete5/concrete5) – so I'm loathe to make
big decisions about github repositories and build workflow while we're
worrying about a major release. When 5.7.1 we'll revisit this.

On Mon, Oct 6, 2014 at 10:36 AM, Fabian Vogler notifications@github.com
wrote:

How would you like us to proceed? Unfortunately I can't put the changes
into a PR as they need the creation of a new GitHub repository as
documented above.


Reply to this email directly or view it on GitHub
#360 (comment)
.

@Remo
Copy link
Contributor Author

Remo commented Oct 6, 2014

@aembler okay, but please let us know if you expect us to "move" anything. This issue is a pretty big deal for us and as Fabian mentioned, we can't do anything at this point.

@ahukkanen
Copy link
Contributor

I'll vote +1 for any progress on this one.

I'm also having hard time to figure out how to manage composer packages in concrete5 packages. Now if there is the same package used in multiple concrete5 packages, there will be possibility for conflicts as many have mentioned in this thread.

Also one note that concrete5 is not the only project struggling with the issue:
https://www.drupal.org/project/composer_manager

It seems that Drupal's solution will also be to move into more "composer-centric architecture".

More on the Drupal's perspective:
https://github.com/cpliakas/composer-manager-docs#why-cant-you-just--

@Remo Remo mentioned this issue Nov 21, 2014
@fabian
Copy link
Contributor

fabian commented Jan 9, 2015

Drupal is moving to a similar solution with a separate drupal-core too btw: composer/installers#208

More detailed information here: https://www.drupal.org/node/1975220

@fabian
Copy link
Contributor

fabian commented Apr 29, 2015

A solution has now been proposed after discussing with the maintainers of composer-installer as well: #2087

@aembler
Copy link
Member

aembler commented Dec 22, 2015

I'm closing this old issue, but this is still something we'd like to see done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants