Fixed public directory when configured in composer.json #29533
Conversation
alexander-schranz
force-pushed
the
bugfix/web-server-public-dir
branch
2 times, most recently
from
f3effd2
to
96c6e41
Compare
alexander-schranz
changed the title
src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php
Outdated
Show resolved
Hide resolved
|
Instead of duplicating the public directory code, I feel like this could be put into a -- edit -- |
|
@nicholasruunu but in which namespace without adding additional requirements to the compoments? |
|
I don't think it's worth adding a new class. |
|
@alexander-schranz @chalasr, True, nevermind. |
|
it could be |
src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php
Outdated
Show resolved
Hide resolved
alexander-schranz
force-pushed
the
bugfix/web-server-public-dir
branch
2 times, most recently
from
3654c68
to
a4ad090
Compare
…ed in composer.json
alexander-schranz
force-pushed
the
bugfix/web-server-public-dir
branch
from
a4ad090
to
c45062b
Compare
|
@nicolas-grekas tests green now :) |
|
|
||
| $composerConfig = json_decode(file_get_contents($composerFilePath), true); | ||
|
|
||
| if (isset($composerConfig['extra']['public-dir'])) { |
There was a problem hiding this comment.
Shouldn't we also account for symfony-web-dir which is still used for application based on the Symfony Standard Edition?
There was a problem hiding this comment.
we discussed about it in #29533 (comment)
wdyt?
There was a problem hiding this comment.
I missed the discussion, but SensioDistributionBundle only handles the case when assets are installed using Composer scripts. IMO being able to run the command without having to specify the argument would be nice. On the other hand, anyone using 3.4 probably is already used to doing this. So maybe it's not worth it.
There was a problem hiding this comment.
anyone using 3.4 probably is already used to doing this. So maybe it's not worth it.
same opinion here :)
| $composerConfig = json_decode(file_get_contents($composerFilePath), true); | ||
|
|
||
| if (isset($composerConfig['extra']['public-dir'])) { | ||
| $publicDir = $composerConfig['extra']['public-dir']; |
There was a problem hiding this comment.
Should we do a ltrim($publicDir, '/') here to be completely sure that we won't have double slashes later?
There was a problem hiding this comment.
is ok for me what do you think @nicolas-grekas?
There was a problem hiding this comment.
we never normalized this var previously: https://github.com/sensiolabs/SensioDistributionBundle/blob/06ec532536f9d6a6049da126c709119176615d45/Composer/ScriptHandler.php#L167-L180
not sure we care, neither we need to.
|
Thank you @alexander-schranz. |
…lexander-schranz) This PR was merged into the 3.4 branch. Discussion ---------- Fixed public directory when configured in composer.json | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT As documented you should be able to change the public-dir by composer.json so the server:run and assets:install should also use that configuration when available: https://symfony.com/doc/3.4/configuration/override_dir_structure.html#override-the-web-directory https://symfony.com/doc/current/configuration/override_dir_structure.html#override-the-public-directory #SymfonyConHackDay2018 Commits ------- c45062b fixed public directory of web server and assets install when configured in composer.json
As documented you should be able to change the public-dir by composer.json so the server:run and assets:install should also use that configuration when available:
https://symfony.com/doc/3.4/configuration/override_dir_structure.html#override-the-web-directory
https://symfony.com/doc/current/configuration/override_dir_structure.html#override-the-public-directory
#SymfonyConHackDay2018