Skip to content
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

CO: Make Symfony routes working even without URL rewriting #5556

Merged

Conversation

@Quetzacoalt91
Copy link
Member

commented May 12, 2016

Questions Answers
Branch? develop
Description? When then URL rewriting is not enabled on Apache, the merchant cannot reach the pages working on Symfony. This PR fixes the issue by adding a missing index.php/ in the URL
Type? Improvement
Category? CO
BC breaks? Nope
Deprecations? Nope
How to test? Disable the URL rewriting on your Apache server, the product and module pages should still be accessible with a modified URL.

@Quetzacoalt91 Quetzacoalt91 changed the title CO: Make links working even without URL rewriting CO: Make Symfony routes working even without URL rewriting May 12, 2016

@Shudrum

View changes

classes/Link.php Outdated
@@ -494,6 +494,10 @@ public function getAdminLink($controller, $with_token = true, $sfRouteParams = a

$params = $with_token ? array('token' => Tools::getAdminTokenLite($controller)) : array();

// Even if URL rewriting is not enabled, the page handled by Symfony must work !
// For that, we add an 'index.php' in the URL before the route
$index = filter_input(INPUT_SERVER, 'HTTP_MOD_REWRITE')?'':'/index.php';

This comment has been minimized.

Copy link
@Shudrum

Shudrum May 13, 2016

Contributor

Can you add some spaces between operators and values?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 May 13, 2016

Author Member

Done

@Shudrum

View changes

classes/Link.php Outdated
@@ -503,25 +507,25 @@ public function getAdminLink($controller, $with_token = true, $sfRouteParams = a
if (!$redirectLegacy) {
if (array_key_exists('id_product', $sfRouteParams)) {
if (array_key_exists('deleteproduct', $sfRouteParams)) {
return $this->getBaseLink().basename(_PS_ADMIN_DIR_).'/product/unit/delete/' . $sfRouteParams['id_product'];
return $this->getBaseLink().basename(_PS_ADMIN_DIR_).$index.'/product/unit/delete/' . $sfRouteParams['id_product'];

This comment has been minimized.

Copy link
@Shudrum

Shudrum May 13, 2016

Contributor

No spaces between the dot operator please :)

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 May 13, 2016

Author Member

Wololo, I'm not responsible of this ! 😛 Done anyway.

@Shudrum

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

👍

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:impr-handle-no-url-rewrite branch May 13, 2016

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented May 14, 2016

Please wait before merge I need to test something

@mickaelandrieu

View changes

classes/Link.php Outdated
@@ -503,25 +507,25 @@ public function getAdminLink($controller, $with_token = true, $sfRouteParams = a
if (!$redirectLegacy) {
if (array_key_exists('id_product', $sfRouteParams)) {
if (array_key_exists('deleteproduct', $sfRouteParams)) {
return $this->getBaseLink().basename(_PS_ADMIN_DIR_).'/product/unit/delete/' . $sfRouteParams['id_product'];
return $this->getBaseLink().basename(_PS_ADMIN_DIR_).$index.'/product/unit/delete/'.$sfRouteParams['id_product'];

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu May 16, 2016

Member

you can use:

$kernel->getContainer()
    ->get('router')
    ->generate('admin_product_unit_action',
        array('action' => 'delete', 'id' => 10),
        true);

Pros:

  • no more hardcoded urls here
  • absolute urls
  • index.php

Cons:

  • index.php will always be "printed" : is that an issue ?
  • Link class now depends on Symfony router

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 May 16, 2016

Author Member

I don't think Link class now depends on Symfony router is a bad thing.

If one of its routes is updated, we won't have to change anything here if the URLs are not hardcoded anymore

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:impr-handle-no-url-rewrite branch 2 times, most recently May 16, 2016

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

Please note this PR has been updated in order to generate URL to Symfony page by using ... the Symfony router.

Regarding

Cons:

  • index.php will always be "printed" : is that an issue ?

I have updated the .htaccess file, which will remove the index.php from the URL and redirect properly the merchant to the cleaner route. 😉

@mickaelandrieu

View changes

classes/Link.php Outdated
// Even if URL rewriting is not enabled, the page handled by Symfony must work !
// For that, we add an 'index.php' in the URL before the route
global $kernel; // sf kernel
$sfRouter = $kernel->getContainer()->get('router');

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu May 16, 2016

Member

what happens if Symfony kernel is not available at this moment ? For instance on installation process ?

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 May 16, 2016

Author Member

Explosion

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:impr-handle-no-url-rewrite branch May 16, 2016

@mickaelandrieu

View changes

classes/Link.php Outdated
// Even if URL rewriting is not enabled, the page handled by Symfony must work !
// For that, we add an 'index.php' in the URL before the route
global $kernel; // sf kernel

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu May 16, 2016

Member

may I suggest something like:

if($kernel instanceof Symfony\Component\HttpKernel\HttpKernelInterface) {
    $sfRouter = $kernel->getContainer()->get('router');
}

This comment has been minimized.

Copy link
@Shudrum

Shudrum May 17, 2016

Contributor

👍

This comment has been minimized.

Copy link
@Quetzacoalt91

Quetzacoalt91 May 17, 2016

Author Member

Here you go

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:impr-handle-no-url-rewrite branch to 6e3f498 May 17, 2016

@mickaelandrieu mickaelandrieu merged commit fd74a58 into PrestaShop:develop May 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Quetzacoalt91 Quetzacoalt91 deleted the Quetzacoalt91:impr-handle-no-url-rewrite branch Jun 14, 2017

@eternoendless eternoendless added this to the 1.7.0.0 milestone Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.