? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
23 Jan 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.

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar laoneo laoneo - open - 23 Jan 2017
avatar laoneo laoneo - change - 23 Jan 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Jan 2017
Category Repository
avatar laoneo laoneo - change - 23 Jan 2017
Labels Added: ?
avatar mbabker
mbabker - comment - 23 Jan 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.

avatar laoneo
laoneo - comment - 23 Jan 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.

avatar mbabker
mbabker - comment - 23 Jan 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.

avatar laoneo
laoneo - comment - 24 Jan 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?

avatar mbabker
mbabker - comment - 24 Jan 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.

avatar laoneo laoneo - change - 30 Jan 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-01-30 07:09:33
Closed_By laoneo
avatar laoneo laoneo - close - 30 Jan 2017
avatar elkuku
elkuku - comment - 27 Apr 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.

avatar elkuku
elkuku - comment - 27 Apr 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.

avatar elkuku elkuku - test_item - 27 Apr 2017 - Tested successfully

Add a Comment

Login with GitHub to post a comment