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

Fixed public directory when configured in composer.json #29533

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
9 participants
@alexander-schranz
Copy link
Contributor

alexander-schranz commented Dec 8, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
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

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:bugfix/web-server-public-dir branch 2 times, most recently from f3effd2 to 96c6e41 Dec 8, 2018

@alexander-schranz alexander-schranz changed the title Fixed public directory of web server when configured in composer.json Fixed public directory when configured in composer.json Dec 8, 2018

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 8, 2018

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Dec 8, 2018

Instead of duplicating the public directory code, I feel like this could be put into a PublicDirectory(ContainerInterface) class.

-- edit --
Or even just PublicDirectory(string projectRoot)

@alexander-schranz

This comment has been minimized.

Copy link
Contributor

alexander-schranz commented Dec 8, 2018

@nicholasruunu but in which namespace without adding additional requirements to the compoments?

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Dec 8, 2018

I don't think it's worth adding a new class.

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Dec 8, 2018

@alexander-schranz @chalasr, True, nevermind.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 8, 2018

it could be %kernel.public_dir%, but that be a new feature also

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

(with one minor comment)

tests are red!

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:bugfix/web-server-public-dir branch 2 times, most recently from 3654c68 to a4ad090 Dec 12, 2018

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:bugfix/web-server-public-dir branch from a4ad090 to c45062b Dec 12, 2018

@alexander-schranz

This comment has been minimized.

Copy link
Contributor

alexander-schranz commented Dec 12, 2018

@nicolas-grekas tests green now :)

$composerConfig = json_decode(file_get_contents($composerFilePath), true);
if (isset($composerConfig['extra']['public-dir'])) {

This comment has been minimized.

@xabbuh

xabbuh Dec 13, 2018

Member

Shouldn't we also account for symfony-web-dir which is still used for application based on the Symfony Standard Edition?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 13, 2018

Member

we discussed about it in #29533 (comment)
wdyt?

This comment has been minimized.

@xabbuh

xabbuh Dec 13, 2018

Member

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 13, 2018

Member

anyone using 3.4 probably is already used to doing this. So maybe it's not worth it.

same opinion here :)

@xabbuh

xabbuh approved these changes Dec 13, 2018

$composerConfig = json_decode(file_get_contents($composerFilePath), true);
if (isset($composerConfig['extra']['public-dir'])) {
$publicDir = $composerConfig['extra']['public-dir'];

This comment has been minimized.

@javiereguiluz

javiereguiluz Dec 13, 2018

Member

Should we do a ltrim($publicDir, '/') here to be completely sure that we won't have double slashes later?

This comment has been minimized.

@alexander-schranz

alexander-schranz Dec 13, 2018

Contributor

is ok for me what do you think @nicolas-grekas?

This comment has been minimized.

@ro0NL

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 13, 2018

Member

why should we care?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 17, 2018

Thank you @alexander-schranz.

@nicolas-grekas nicolas-grekas merged commit c45062b into symfony:3.4 Dec 17, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 17, 2018

bug #29533 Fixed public directory when configured in composer.json (a…
…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

@alexander-schranz alexander-schranz deleted the alexander-schranz:bugfix/web-server-public-dir branch Dec 17, 2018

This was referenced Jan 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment