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

Manage backward compatibility of legacy links #10868

Merged
merged 17 commits into from Oct 11, 2018

Conversation

Projects
None yet
7 participants
@jolelievre
Contributor

jolelievre commented Oct 4, 2018

Questions Answers
Branch? 1.7.5.x
Description? Manage backward compatibility of legacy links, see #10745 for more detail
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10811 #10745 #10804 #10817
How to test? To test this new feature you need to select this PR branch of course. Then the easiest way to test it is to use this module https://github.com/jolelievre/testlegacyurl
It displays all the migrated links and allows you to see if they have been correctly converted
You can also click on the legacy links to check if the redirection is correctly done
If you don't want to clone the module repository you can download it here testlegacyurl.zip

This change is Reviewable

$legacyLink = $routeDefaults['_legacy_link'];
$linkParts = explode(':', $legacyLink);
$legacyController = $linkParts[0];
$legacyAction = count($linkParts) > 1 ? $linkParts[1] : null;

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 5, 2018

Contributor

isset($linkParts[1]) is maybe faster than count?

This comment has been minimized.

@jolelievre

jolelievre Oct 5, 2018

Contributor

good question, I can change it anyway and even use empty to be sure
But regarding the performance.. do you know which one is faster?

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 5, 2018

Contributor

From what I read, isset is maybe one of the fastest function of php. it doesn't need loop or anything.

/** @var Route $route */
foreach ($this->router->getRouteCollection() as $routeName => $route) {
$this->addRoute($routeName, $route);
}

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Oct 5, 2018

Member

How long does it take to run this loop?

If the time needed is noticeable. It could be better to run it at the first real use of this class.

This comment has been minimized.

@jolelievre

jolelievre Oct 5, 2018

Contributor

I don't think it is that long since it only creates a few arrays, but I can delay this creation via getters sure, it's probable for the best
Although since this service is called pretty early in the Listener it will have to perform it anyways, so I think the gain will be small

@Quetzacoalt91 Quetzacoalt91 changed the title from WIP: Manage backward compatibility of legacy links to Manage backward compatibility of legacy links Oct 5, 2018

Show resolved Hide resolved src/Core/Exception/CoreException.php
*/
class PrestaShopCoreException extends \Exception
class ErrorMessageException extends CoreException

This comment has been minimized.

@matks

matks Oct 5, 2018

Contributor

Isn't it rather a TranslationAwareException as suggested by @sarjon in #10865 (comment) ?

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

yep good idea, although I don't like TranslationAwareException because it gives the feeling that the Exception is capable of translating itself, as if it contained the translator service
I prefered TranslatableCoreException which seems more correct to me

prestashop.bundle.routing.legacy_url_converter:
class: 'PrestaShopBundle\Routing\LegacyUrlConverter'
arguments:
$router: '@router'

This comment has been minimized.

@matks

matks Oct 5, 2018

Contributor

😮 I did not know this syntax for SF services
👍

This comment has been minimized.

@PierreRambaud
namespace PrestaShopBundle\Routing\Exception;
use PrestaShop\PrestaShop\Core\Exception\ErrorMessageException;
use Throwable;

This comment has been minimized.

@matks

matks Oct 5, 2018

Contributor

As found by sarjon in #10865 : careful with Throwable we still have to be php5.6-compatible

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

ok, I integrated the modif but can you check if it is good TranslatableCoreException
Especially the use part, is it ok to have use Throwable in php 5.6 or should I use \Throwable and \Exception instead?

This comment has been minimized.

@matks

matks Oct 9, 2018

Contributor

I tried, it seems the use is not a issue for php5.6

*/
public function __construct($key, array $parameters = [], $domain = null, $code = 0, Throwable $previous = null)
{
parent::__construct($key, $domain, $parameters, $code, $previous);

This comment has been minimized.

@matks

matks Oct 5, 2018

Contributor

Why do you override the __construct if you copy the parent behavior ?

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 5, 2018

Contributor

it change $parameters and $domain order, but don't know why :/

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

be cause I didn't have a domain for this exception, but it is a bad idea to leave it empty, and this kind of exception is not displayed as an error message or notification
So I changed it to a more common CoreException

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 5, 2018

Woot, what a work 👍

@@ -41,6 +44,17 @@ admin_module_manage:
keyword:
_controller: PrestaShopBundle:Admin/Improve/Module:manage
_legacy_controller: AdminModulesManage
_legacy_link: AdminModulesManage
admin_module_manage_alias:

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 9, 2018

Contributor

make _legacy_link a possible array for aliases will be better, no?

_legacy_link:
  - AdminModuleManager
  - AdminModulesSf

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

yeah, I'm gonna add this feature

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

done!

_controller: PrestaShopBundle:Admin/Improve/Module:manage
_legacy_controller: AdminModulesManage
_legacy_link: AdminModulesSf
_legacy_link:

This comment has been minimized.

@PierreRambaud

PierreRambaud Oct 9, 2018

Contributor

👍

@@ -32,12 +32,15 @@
use Link;
use ReflectionClass;
/**
* @group demo

This comment has been minimized.

@sarjon

sarjon Oct 9, 2018

Member

offtopic question, but how do you know which group it belongs to? 🤔 is there any docs regarding it?

This comment has been minimized.

@jolelievre

jolelievre Oct 9, 2018

Contributor

well.. I just found out about these groups 😄
so I used one of those defined in phpunit-admin.xml because it was the only way to make my tests work.. (not very proud of this 🤫)

This comment has been minimized.

@sarjon

sarjon Oct 9, 2018

Member

@matks we could improve it. :)

This comment has been minimized.

@matks

matks Oct 9, 2018

Contributor

Yes we can !

@matks matks added the waiting for QA label Oct 9, 2018

Obsolete

@marionf marionf self-assigned this Oct 10, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Oct 10, 2018

@jolelievre

I have an exception when I click on Design > Theme & logo > Theme custo

capture d ecran_421

When I click on Shop parameters > Traffic & SEO > Add new page or edit, I have this:

capture d ecran_422

I have also these exceptions when I click on some links in your module, but maybe it's normal ?

capture d ecran_423

capture d ecran_424

capture d ecran_425

@marionf marionf removed their assignment Oct 10, 2018

@PierreRambaud PierreRambaud dismissed stale reviews from matks and themself via 7354001 Oct 10, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 10, 2018

@marionf I fix issues and it's normal for these links. (Allow only in POST / DELETE)

@PierreRambaud PierreRambaud merged commit 119d581 into PrestaShop:1.7.5.x Oct 11, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 11, 2018

Thanks @jolelievre

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