modxcms / revolution

MODX Revolution - Content Management Framework
https://modx.com/
GNU General Public License v2.0
1.36k stars 529 forks source link

Create composer.json #13619

Closed johanmolen closed 6 years ago

johanmolen commented 7 years ago

What does it do ?

Turns MODX into Composer library. No autoloading by namespaces, no core split into separate packages - just making MODX available to be installed via Composer, nothing more and nothing less.

Why is it needed ?

It gives MODX an opportunity to be installed via the most popular dependency manager for PHP. It wont be on packagist by default but we can at least require MODX with Composer using repositories. This won't have any disadvantages.

Related issue(s)/PR(s)

Thanks to @PatchRanger issue #12422.

And pull request #13620.

OptimusCrime commented 7 years ago

https://github.com/modxcms/revolution/pull/12422#issuecomment-172581023

johanmolen commented 7 years ago

@OptimusCrime I know... but what is the disadvantage of doing it? It can improve our workflow a lot and it doesn't have any disadvantages for MODX. If you have good arguments please tell me, instead of linking to a comment which has no arguments at all....

OptimusCrime commented 7 years ago

Other than the fact that the MODX people would have to manage the Packagist package, nothing I guess.

However, I am not really sure how it would work when the Revolution code does not adhere to any autoloading what so ever? Can this create problems with composer?

Mark-H commented 7 years ago

I'm not sold on the why bit yet. It doesn't allow MODX to be installed as modx has a separate setup process, at most it allows to download the MODX files, which people would then need to manually build (as if installing from git) before they can run setup.

Is that really an improved workflow?

johanmolen commented 7 years ago

It is not a requirement to be on Packagist we can still require MODX using other composer methods.

I think that is a matter of configuration. They managed to do it with WordPress, for example: https://github.com/roots/bedrock

johanmolen commented 7 years ago

@Mark-H why manual? We could create our own scripts for it right?

PatchRanger commented 7 years ago

@Mark-H To extend @pixelive comment: even more - from MODX-styled scripts they will turn into declarative composer-scripts, which are much more standard.

MODX to be installed as modx has a separate setup process

The whole setup process could (and in my view - should) be turned into composer scripts. Ideally composer install should be enough to build entire system from scratch - without additional manual handling or custom actions.

Mark-H commented 7 years ago

@patchranger I agree, that would be amazing, but that's something for 3.x, which has already started using a composer file and autoloading for xPDO, and not 2.x.

johanmolen commented 7 years ago

@PatchRanger True! That is an awesome feature but we should start with adding this file 😄

johanmolen commented 7 years ago

@Mark-H @PatchRanger maybe someone in the community is willing to share this feature/change/scripts if we just add the composer.json from now on?

gpsietzema commented 7 years ago

Someone please correct me if I'm wrong: (1) This addition will make it possible to get all MODX files using composer install. The advantage of this is developers can setup MODX using just composer, instead of downloading/cloning MODX. This will lessen the size of MODX-website-repositories. (2) This addition has completely nothing to do with the work which is currently being done on making all components (xpdo / core / whatever) composer packages. It does not interfere with it at all. (3) This composer file can be replaced by the one mentioned in (2) when it's finished. (4) Updating isn't part of this. We'll be making our own scripts for this and you'll probably see these coming through a PR pretty soon.

After taking in these 4 points, we may conclude that we can simply integrate this, with a small addition to the README.

Correct?

rthrash commented 7 years ago

This totally makes sense to me for 2.6.

rthrash commented 7 years ago

Gah. Mobile app got me:

This totally makes sense to me for 2.6. For MODX developers who use Composer, and I would assume Sterc is one of them based in the source, it would be a solid win. One that can be interated and improved upon over time that doesn’t seem to have any downside. A practical win if you will.

Mark-H commented 7 years ago

When you add something like this, it would inevitably be presented as "MODX can now be installed with composer" by someone, which is simply not true.

Adding this composer file, with no further changes, is IMO simply a (worse) alternative to the existing download options. You would still need to build the core and run the setup, as if it were an install from git, but do not get any of the benefits that a git build would give you (the ability to switch branches and even different remotes, or to commit changes to the repo so they can be upstreamed, for example).

Maybe composer install modxcms/revolution is slightly easier to remember than git clone git@github.com:modxcms/revolution.git? If you're going to need custom scripts to, you know, install MODX after a composer install, why don't you just add the clone (or wget https://modx.com/download/latest - so you can skip the build step) to those scripts?

@gpsietzema

Point 1): the "developers can setup MODX using just composer" is simply not true, as you still need your own scripts to actually be able of installing it.

Point 2) and 3), while it is indeed separate, it will actually interfere as it will make merges from 2.x to 3.x more complex. Especially if, as suggested, we start with this and build upon it to make it better. Suddenly you have 2 distinct version of the same JSON file with no shared history, as 3.x already has a composer.json for autoloading and a few dependencies. That's going to be fun!

Point 4): you're probably going to get the latest files whenever you do a composer update, so seeing it as "not part of this" isn't exactly fair. If you're going to allow downloads (/installs), people will also expect updates. Aside from the huge ecosystem, the ability to easily upgrade is one of the key reasons I enjoy working with composer on a daily basis, but that's not going to work without significant custom code for this proposal.

Wouldn't this also download MODX into the vendor directory? That doesn't sound like a great developer experience to me at all. Perhaps a composer create-project integration would be a much better alternative, but I don't think that has to necessarily be part of the official repo.


Maybe I'm bikeshedding over an arguably small change, and I want to make clear that I'm all for improving the developer experience in setting up MODX and making more use of tools like composer/packagist and the benefits that come with that, but all I see here is a glorified git clone that does not meet what people's expectations for it might be when they hear you can "install from composer now".

With the presented arguments in this thread and my slack chat with Johan yesterday, I am still not in favour of this change. You may be able to convince me by explaining how this would affect your workflow in a way that you can't already do in the custom scripts you'll need to get it running anyway.


Let's make MODX actually installable from composer, starting with 3.x. That is what I would call a "solid win".

(Note: this is my personal opinion, not an official MAB or integrators perspective on the topic)

OptimusCrime commented 7 years ago

I think you can chose where to place a package when you do composer require, other than that I agree 100% with you @Mark-H.

opengeek commented 6 years ago

I'm 100% in agreement with @Mark-H here. The expectation would be if I composer install MODX, it should be ready to use. As is, you still have to go through all the build steps outlined at https://docs.modx.com/revolution/2.x/getting-started/installation/git-installation to get it working afterward. Let's work together to make MODX 3.x completely installable from Composer via Packagist and get rid of the dependencies it currently has on being built for distribution.

gpsietzema commented 6 years ago

This whole discussion was all-negative. People just giving reasons why not to do that, because it doesn’t add anything to their workflow. Apart from the fact that it doesn’t hurt their workflow either. Feels a bit like politicians who are in the opposition and just have an argument, because they can. The only logical response from the Pull Requester in these cases is: “Screw this, I’ll never do a PR again. I’ll just make my own shit.“.

Jason’s comment is how it should be to be honest: work together on modifying this to fit more people’s workflow. We created this from the agency-who-makes-a-lot-of-big-websites-with-lots-of-people-and-other-agencies-working-on-it-perspective and if we need to widen our perspective to make it work for more people, then we’d be open to that, but it will not work if we only get a waterfall of shit over our heads.

My two cents…

For now: we’ll continue working like this and make our composer available somewhere in order for other agencies to use it. We’d love help with the 3-composer thing.

alroniks commented 6 years ago

I don't understand opponents of adding composer.json. I don't know who will be shout that now MODX supports composer installation. And composer itself has a lot of internal functions (not only checkout code from a repository), that allows to install code in any folder and run any scripts after events. I would say that it possible to create the composer-based installer for MODX. Why not use this possibility when people already have some experience with that? It also may help with such composer file for 3.x too. Yes, It would be good to give all efforts to developing 3.x, but full development process still not started and a lot of people will use 2.x a long time.

opengeek commented 6 years ago

The opposition is pretty well described above. Honestly, this back and forth is ridiculous. The 3.x branch has been there over a year waiting for contributions yet there have been almost none made to it.

opengeek commented 6 years ago

Innovation like this is best created outside the repository. That is all that was being suggested here, perhaps in not so many words. If you want to work on the scripts to make it installable directly from Composer, please do. And then we can evaluate those, refine them if necessary, and get them properly integrated into the repository at whatever point makes sense. But without those, this doesn't appear to be useful to a general audience of MODX consumers. That is the issue. There are consequences to everything we integrate into the MODX core repository and we subsequently take those integrations very seriously. No one is trying to create a waterfall of shit, we simply do not want to prematurely merge changes into the public core repository without a complete solution in place. Many points of this discussion are referencing the existing pull request which was submitted along with the creation of this issue, which does not include scripts to accomplish the installation.

alroniks commented 6 years ago

I agree with you @opengeek, but for me strange that first draft was perceived so negatively. And I can understand GP, because they started to investigate composer issue and shared first results and after that, they were rejected, sometimes rough rejected.

About MODX 3, as I know we have one the main unresolved issue with inheriting xpdo by modx and I saw a lot of discussions about it in Slack, it's not an easy question. OptimusCrime proposed some ideas, there was discussed even "crazy" idea about starting from scratch and broke BC. But we need one clear decision once and for all.

I do not blame anyone, we are all enthusiasts here to some extent. But we need speak more and discuss more against to start wars about what we should do or not.

opengeek commented 6 years ago

I don't understand. There were clearly discussed objections in this issue. Not once did anyone provide evidence of how this improved anyone's workflow.

And that discussion you are referencing has nothing to do with 3.x. 3.x has been there and usable for over 2 years actually. The Slim refactor is a separate issue and 3.x is NOT dependent on that work, at all.

opengeek commented 6 years ago

Also, the composer issue has been discussed in several different PRs since 2014. It was added for 3.x over two years ago. Maybe that helps clarify where any perception of frustration is coming from on this side of the issue.

opengeek commented 6 years ago

See #12422, #12092, #12063 for additional history on this.

Mark-H commented 6 years ago

@gpsietzema and @pixelive: I apologize if my concerns with the proposed PR came across as too negative. It's not at all my intention to discourage contributions or to make contributing feel like a "waterfall of shit" and I'm genuinely sorry if my actions contributed to feeling like that.

I do very much appreciate the work you and Sterc do for MODX. Hopefully, the discussions here will lead to some solid improvements to the developer/setup experience that everyone gets excited about.

OptimusCrime commented 6 years ago

My five cents is to make a push for 3.x instead of the endless 2.x versions. We should focus our attention on getting 3.x ready with the desired features. I have a small project going now, but as soon as I've finished it, I will look at how we can make the external dependencies Composer dependencies instead.

Mark-H commented 6 years ago

I'm closing this as #13784 created actual objective improvements to the git installation process by automating the build steps with a composer create-project. It's available on the 2.x branch but also 3.x as the setup processes are the same for both.