[HttpFoundation] optimize files for distribution #24427

jpuck · 2017-10-04T21:14:24Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

Currently every production installation will pull in all of the tests into the vendor directory. This is unnecessary file bloat on the server and network bandwidth. By adding a .gitattributes file with filenames listed to be excluded during export, this will speed up installations and facilitate better stewardship of server resources.

This is my 1st PR, so I hope I understood the contributing guidelines and submitted to the correct repository and branch, so please kindly advise if otherwise. If acceptable in principle, then I'd be happy to duplicate the efforts in additional commits for the other components. I chose the HttpFoundation subtree because it came to my attention as it was used in a project when a lint failed on a php 5.6 server only because of php 7 language used in one of the tests.

Thanks!

Tobion · 2017-10-04T21:17:25Z

https://github.com/symfony/symfony/search?q=gitattributes&type=Issues&utf8=%E2%9C%93

jpuck · 2017-10-04T21:19:56Z

@Tobion ah thanks for the reading material. So should I just close now then?

javiereguiluz · 2017-10-05T06:28:24Z

@jpuck it's true that this has been asked for (and rejected) many times before, but it's OK to discuss about this again from time to time. Besides, if so many people and so many times ask for the same thing, maybe it's time to reconsider our past decision. Thanks!

linaori · 2017-10-05T06:39:55Z

I'm still of opinion that it's 100% useless to include tests unless you specifically install the source as dependency. I don't care about the amount of files, or even that it's in production, but I do care about the DX of developing with Symfony. Current DX status when looking up classes and implementations:

Stubs polluting my search results
There are Test and Tests namespaces, where the former is a public API to extend while the latter is not, caused me a "wtf" moment more than once already

The biggest issue is that all searches are heavily polluted with search results from tests:

While everything is being done to enhance the DX of Symfony, this is still a big issue.

Tobion · 2017-10-05T07:05:06Z

I full agree wit @iltar and I've alwas been in favor in adding this.

sroze · 2017-10-05T08:26:02Z

Definitely agree with this as well, these tests files shouldn't be within the "distributed archive". Ideally I'd move them to tests/ instead of src/ within an autoload-dev but I guess we can't do this as it might cause a BC break. I guess the .gitattributes option is a good compromise 👍

linaori · 2017-10-05T08:28:56Z

The problem is primarily present when you use symfony/symfony, so with flex the problem becomes smaller. I would love to have a test directory of sorts that's on autoload-dev only, makes it really easy to exclude tests and will always prevent you from adding tests either way.

nicolas-grekas · 2017-10-05T08:33:17Z

Having tests in each component is a requirement so there is no way to move tests outside of src, unless someone wants to take the responsibility and has the leadership to rewrite all our processes and releasing infrastructure.

About DX, I think https://youtrack.jetbrains.com/issue/WI-37750 will do the job.
Of course, that'll require adding @internal to all test classes. PR welcomed.

👎 on my side, with the same rationale as previously detailed.

afurculita · 2017-10-05T08:48:34Z

For symfony/symfony we can add the paths to all the Tests folders (or a regex) in a .gitattributes file at the root of the project.

linaori · 2017-10-05T08:59:05Z

That means all tests would have to be marked with @internal?

nicolas-grekas · 2017-10-05T09:01:53Z

@iltar yes. That looks accurate to me, anything in "Tests" folder is out of BC policy, so they are internal.

linaori · 2017-10-05T09:04:44Z

But that won't fix it for:

Searches
People that don't use PhpStorm

nicolas-grekas · 2017-10-05T09:08:38Z

It's still an IDE-domain problem.
A more generic solution would be: editorconfig/editorconfig#228
(ref for previous discussion btw: #17749)

linaori · 2017-10-05T09:09:20Z

Excluding the tests from dist files would cover all scenarios though, no opt-outs for anything, no additional work for the developer

nicolas-grekas · 2017-10-05T09:11:39Z

Not all usages, as previously discussed in #17749.

linaori · 2017-10-05T09:17:27Z

That was a slightly different topic, moving the files. The only thing I would like to see this issue resolve in, is no test files present when I add symfony as a dependency to my project.

I have yet to see an argument that might convince me that this is a bad idea for the majority of people. Adjusting the IDE or config files for every single developer is a lot of work, where as the minuscule amount of developers that does want the test files, can get them via numerous ways.

Why not hold a poll whether or not the community wants the test files included or excluded from the dist?

nicolas-grekas · 2017-10-08T16:57:45Z

Closing as explained.

…dist" (Nyholm) This PR was merged into the 4.4 branch. Discussion ---------- Adding .gitattributes to remove Tests directory from "dist" | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | Deprecations? | no? | Tickets | | License | MIT | Doc PR | This is a controversial topic that have been mentioned before. We recently had some discussions on Slack about it and the community not in an agreement. This was asked back in 2014 already. Im making this PR again, because I think this will help more people than it hurts to keep the tests in the "dist" version. ### Reasons for keeping the tests with the source * You can look at the tests to understand how the code works * It is convenient In the past there were an argument of people might depend on Symfony's classes in Tests. That is no longer the case since we moved reusable classes from Tests to Test. ### Reasons for removing them (merging this PR) * There should be difference between `composer update --prefer-source` and `composer update --prefer-dist` * Smaller packages when deploying with Docker or on Serverless. * Static analysis tools will not complain on PHP syntax errors in our tests ([example](https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/xml_with_wrong_ext.php)) ## How to decide? Merging this PR or not is tricky because no side has a solid technical argument. It is basically just personal preference. Please give this PR a 👍 or 👎 if you want to give your opinion. ## Other PRs and issues related to this: Add .gitattributes file (#29277) Added .gitattributes files to root and all components (#26472) Exclude non-essential files from Composer package (#25414) [HttpFoundation] optimize files for distribution (#24427) Add .gitattributes files (#23926) [Suggestion] Adding .gitattributes to ignore unnecessary folders and files for production env (#20057) Add lightweight and root only .gitattributes (#18004) Add .gitattributes to exclude tests from ZIPs (#17995) [RFC] Move tests out of the source and source out of the tests (#17749) Removal of development & testing files using .gitattributes (#16174) Please add .gitattributes files and fix line endings (#13521) making use of .gitattributes (#11810) ## Workarounds There are workarounds for both sides. Example: ### Workaround if merged * `composer update --prefer-source` ### Workaround if closed * `find vendor/symfony -name "Tests" -type d -exec rm -r "{}" \;` * editorconfig/editorconfig#228 * https://github.com/dg/composer-cleaner Commits ------- ac7dc24 Adding .gitattributes to remove Tests directory from "dist"

…dist" (Nyholm) This PR was merged into the 4.4 branch. Discussion ---------- Adding .gitattributes to remove Tests directory from "dist" | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | Deprecations? | no? | Tickets | | License | MIT | Doc PR | This is a controversial topic that have been mentioned before. We recently had some discussions on Slack about it and the community not in an agreement. This was asked back in 2014 already. Im making this PR again, because I think this will help more people than it hurts to keep the tests in the "dist" version. ### Reasons for keeping the tests with the source * You can look at the tests to understand how the code works * It is convenient In the past there were an argument of people might depend on Symfony's classes in Tests. That is no longer the case since we moved reusable classes from Tests to Test. ### Reasons for removing them (merging this PR) * There should be difference between `composer update --prefer-source` and `composer update --prefer-dist` * Smaller packages when deploying with Docker or on Serverless. * Static analysis tools will not complain on PHP syntax errors in our tests ([example](https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/xml_with_wrong_ext.php)) ## How to decide? Merging this PR or not is tricky because no side has a solid technical argument. It is basically just personal preference. Please give this PR a 👍 or 👎 if you want to give your opinion. ## Other PRs and issues related to this: Add .gitattributes file (symfony/symfony#29277) Added .gitattributes files to root and all components (symfony/symfony#26472) Exclude non-essential files from Composer package (symfony/symfony#25414) [HttpFoundation] optimize files for distribution (symfony/symfony#24427) Add .gitattributes files (symfony/symfony#23926) [Suggestion] Adding .gitattributes to ignore unnecessary folders and files for production env (symfony/symfony#20057) Add lightweight and root only .gitattributes (symfony/symfony#18004) Add .gitattributes to exclude tests from ZIPs (symfony/symfony#17995) [RFC] Move tests out of the source and source out of the tests (symfony/symfony#17749) Removal of development & testing files using .gitattributes (symfony/symfony#16174) Please add .gitattributes files and fix line endings (symfony/symfony#13521) making use of .gitattributes (symfony/symfony#11810) ## Workarounds There are workarounds for both sides. Example: ### Workaround if merged * `composer update --prefer-source` ### Workaround if closed * `find vendor/symfony -name "Tests" -type d -exec rm -r "{}" \;` * editorconfig/editorconfig#228 * https://github.com/dg/composer-cleaner Commits ------- ac7dc24bcb Adding .gitattributes to remove Tests directory from "dist"

carsonbot added Status: Needs Review HttpFoundation labels Oct 4, 2017

jpuck force-pushed the export-ignore-tests branch from 12292c6 to 3ff1d58 Compare October 4, 2017 21:22

[HttpFoundation] optimize files for distribution

3ff1d58

nicolas-grekas closed this Oct 8, 2017

Nyholm mentioned this pull request Sep 14, 2019

Adding .gitattributes to remove Tests directory from "dist" #33579

Merged

Uh oh!

[HttpFoundation] optimize files for distribution #24427

[HttpFoundation] optimize files for distribution #24427

Uh oh!

Conversation

jpuck commented Oct 4, 2017

Uh oh!

Tobion commented Oct 4, 2017

Uh oh!

jpuck commented Oct 4, 2017

Uh oh!

javiereguiluz commented Oct 5, 2017

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

Tobion commented Oct 5, 2017

Uh oh!

sroze commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

afurculita commented Oct 5, 2017

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

linaori commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 5, 2017

Uh oh!

linaori commented Oct 5, 2017

Uh oh!

nicolas-grekas commented Oct 8, 2017

Uh oh!

Uh oh!

sroze commented Oct 5, 2017 •

edited

Loading

linaori commented Oct 5, 2017 •

edited

Loading