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

Ignore developer packages #13701

Closed
wants to merge 2 commits into from

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Jan 23, 2017

When doing actually in staging a composer install all the new developer packages are showing up in git status. This ignore additions will hide them.

Basically a maintainer review.

@mbabker
Copy link
Contributor

mbabker commented Jan 23, 2017

There is a part of me that wants to say to not do this. Until this repo uses Composer The One Right Way™ the repo has to have only the production dependencies checked in. If you add those paths to the gitignore, it becomes far too easy to screw up and check in the autoloader with dev dependencies.

composer install
git status

On branch staging
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   libraries/vendor/composer/autoload_classmap.php
	modified:   libraries/vendor/composer/autoload_files.php
	modified:   libraries/vendor/composer/autoload_namespaces.php
	modified:   libraries/vendor/composer/autoload_psr4.php
	modified:   libraries/vendor/composer/autoload_real.php
	modified:   libraries/vendor/composer/autoload_static.php
	modified:   libraries/vendor/composer/installed.json

For me having all of those extra paths in the git status output reminds me I need to run composer install --no-dev before committing.

@laoneo
Copy link
Member Author

laoneo commented Jan 23, 2017

Agree, but during development, when you want to locally commit stuff, you don't just want to make an install --no-dev first, commit the stuff and then do a composer install again.

@mbabker
Copy link
Contributor

mbabker commented Jan 23, 2017

Well then this project needs to stop having this repository in a state where it can be cloned and installed with zero effort. That's why the vendor directory is checked in, it is viewed as too much effort to suggest contributors might need to run a composer install first to get a functional site or to be able to contribute a patch.

So with that said, there should be no point in a commit log that the autoloader contains the development dependencies. As annoying as it is, you do have to go through the install --no-dev, commit, install steps to do it right.

@laoneo
Copy link
Member Author

laoneo commented Jan 24, 2017

Would it be an option to check during install if the vendor directory is available, when not a composer install is done. Like that we can remove it in github but ship it with the download packages of joomla.org?

@mbabker
Copy link
Contributor

mbabker commented Jan 24, 2017

In general, no.

composer install shouldn't be triggered from a web request, it WILL time out unless you've got a high memory PHP install and the timeouts disabled. This also assumes people either have Composer installed or their systems allow system(), exec(), or stuff like that to be executed. And some of the core dependencies are inheriting from the vendor directory in a way that makes booting the app next to impossible, it would have to be a very early abort (about the same point as the PHP minimum check, so the second functional statement executed) and a "you don't have dependencies installed" message.

Not to mention it breaks the workflow a lot use of downloading the PR branch from GitHub versus using git to apply patches. This isn't a knock on anyone, but there is a high number of contributors who don't make consistent use of tools like git, Composer, or anything else you'd run on a command-line interface.

@laoneo laoneo closed this Jan 30, 2017
@laoneo laoneo deleted the ignore-dev-packages branch January 30, 2017 07:09
@elkuku
Copy link
Contributor

elkuku commented Apr 27, 2017

Bad stuff :(
I think we should implement a voting feature so people can vote for stuff like this.
Well actually we have it but nobody uses it. FWIW I just voted for this "bug" 😉


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13701.

@elkuku
Copy link
Contributor

elkuku commented Apr 27, 2017

I have tested this item ✅ successfully on 13520a9

I've tested this even if the PR had been closed 👅


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13701.

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

Successfully merging this pull request may close these issues.

4 participants