Skip to content

[FrameworkBundle] Fix paths with % in it (like urlencoded) #17307

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 11, 2016
Merged

[FrameworkBundle] Fix paths with % in it (like urlencoded) #17307

merged 1 commit into from
Jan 11, 2016

Conversation

scaytrase
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? Travis passed
Fixed tickets #17306
License MIT
Doc PR none

@scaytrase scaytrase changed the title Fix paths with % in it (like urlencoded) [FrameworkBundle] Fix paths with % in it (like urlencoded) Jan 8, 2016
@dosten
Copy link
Contributor

dosten commented Jan 8, 2016

This is a BC break, you need to manually escape the % char

@scaytrase
Copy link
Contributor Author

@dosten How should this be a BC break? Kernel parameter % signs are resolved before. And all other % should be passed as is to the sprintf. In other case they cause Exception (see the bug attached)

@scaytrase
Copy link
Contributor Author

i.e if we have path /var/www/some%2Fproject/ (which is '/var/www/' . urlencode('some/project')) we will have the exception. Passign % as is should not be a BC break as sprintf expects only one %s template in the path.

@Tobion
Copy link
Contributor

Tobion commented Jan 9, 2016

The better fix is to only apply the sprintf to /Resources/%s/translations and not the kernel root dir. This way there is no need to encode anything.

@scaytrase
Copy link
Contributor Author

@Tobion Yes, and this would have more clear logic with sprifntf.

Does this one looks better?

Tobion
Tobion reviewed Jan 9, 2016
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -562,13 +562,13 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
$dirs[] = dirname($r->getFileName()).'/../../Resources/translations';
}
}
$overridePath = $container->getParameter('kernel.root_dir').'/Resources/%s/translations';
$rootDir = $container->getParameter('kernel.root_dir');
Copy link
Contributor

Choose a reason for hiding this comment

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

line 575 can reuse $rootDir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tobion updated

@Tobion
Copy link
Contributor

Tobion commented Jan 9, 2016

👍

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Jan 11, 2016

Thank you @scaytrase.

@fabpot fabpot merged commit de7b4d7 into symfony:2.3 Jan 11, 2016
fabpot added a commit that referenced this pull request Jan 11, 2016
@fabpot
bug #17307 [FrameworkBundle] Fix paths with % in it (like urlencoded)…
a02bd4b 
… (scaytrase)

This PR was merged into the 2.3 branch.

Discussion
----------

[FrameworkBundle] Fix paths with % in it (like urlencoded)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | Travis passed
| Fixed tickets | #17306
| License       | MIT
| Doc PR        | none

Commits
-------

de7b4d7 Fix #17306 Paths with % in it are note allowed (like urlencoded)
@scaytrase scaytrase deleted the 2.3 branch January 11, 2016 10:09
@scaytrase scaytrase restored the 2.3 branch January 11, 2016 10:09
This was referenced Jan 14, 2016
Merged
Merged
Merged
@fabpot fabpot mentioned this pull request Feb 3, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@scaytrase @dosten @Tobion @fabpot @carsonbot