Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fixed public directory when configured in composer.json#29533

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@alexander-schranz
Copy link
Contributor

@alexander-schranzalexander-schranz commentedDec 8, 2018•
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#...
LicenseMIT

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-schranzalexander-schranzforce-pushed thebugfix/web-server-public-dir branch 2 times, most recently fromf3effd2 to96c6e41CompareDecember 8, 2018 16:08
@alexander-schranzalexander-schranz changed the titleFixed public directory of web server when configured in composer.jsonFixed public directory when configured in composer.jsonDec 8, 2018
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneDec 8, 2018
@nicholasruunu
Copy link
Contributor

nicholasruunu commentedDec 8, 2018•
edited
Loading

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

-- edit --
Or even justPublicDirectory(string projectRoot)

@alexander-schranz
Copy link
ContributorAuthor

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

@chalasr
Copy link
Member

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

@nicholasruunu
Copy link
Contributor

@alexander-schranz@chalasr, True, nevermind.

@ro0NL
Copy link
Contributor

ro0NL commentedDec 8, 2018•
edited
Loading

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

nicolas-grekas
nicolas-grekas previously approved these changesDec 12, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(with one minor comment)

@alexander-schranzalexander-schranzforce-pushed thebugfix/web-server-public-dir branch 2 times, most recently from3654c68 toa4ad090CompareDecember 12, 2018 17:55
@alexander-schranz
Copy link
ContributorAuthor

@nicolas-grekas tests green now :)


$composerConfig =json_decode(file_get_contents($composerFilePath),true);

if (isset($composerConfig['extra']['public-dir'])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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'];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
ContributorAuthor

@alexander-schranzalexander-schranzDec 13, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

why should we care?

ro0NL and javiereguiluz reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

Thank you@alexander-schranz.

@nicolas-grekasnicolas-grekas merged commitc45062b intosymfony:3.4Dec 17, 2018
nicolas-grekas added a commit that referenced this pull requestDec 17, 2018
…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       | MITAs 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-directoryhttps://symfony.com/doc/current/configuration/override_dir_structure.html#override-the-public-directory#SymfonyConHackDay2018Commits-------c45062b fixed public directory of web server and assets install when configured in composer.json
@alexander-schranzalexander-schranz deleted the bugfix/web-server-public-dir branchDecember 17, 2018 10:13
This was referencedJan 6, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@xabbuhxabbuhxabbuh approved these changes

+2 more reviewers

@ro0NLro0NLro0NL left review comments

@KronhyxKronhyxKronhyx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

9 participants

@alexander-schranz@nicholasruunu@chalasr@ro0NL@nicolas-grekas@javiereguiluz@xabbuh@Kronhyx@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp