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

Change brand and supplier default rule for SEO purposes #9458

Merged
merged 1 commit into from Aug 30, 2018

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Aug 16, 2018

Questions Answers
Branch? develop
Description? Change brand and supplier default rule for SEO purposes, brand url is brand/IDOFBRAND-BRANDNAME, supplier url is supplier/IDOFSUPPLIER-SUPPLIER
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-6258
How to test? Go to the site map to find brand links and check that the urls use the new format

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.5.0 milestone Aug 16, 2018
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Aug 16, 2018
@PierreRambaud
Copy link
Contributor

We need to add a way to add 301 from old urls.

@eternoendless
Copy link
Member

eternoendless commented Aug 17, 2018

I'm not very fond of this solution for redirection. The need is real though, because once you change the setting in the BO then it should add a permanent route that redirects to the new one, and same for every change. That is a more complex issue that should be handled in a separate PR.

In the meantime, I suggest keeping the solution we discussed about for current shops (writing the current route in DB during the upgrade so that their URLs don't change).

@jolelievre
Copy link
Contributor Author

I agree with you about the url in database which would be better, but I didn't find a way to do it with database changes only.
The thing is you can't exactly add a route alias in the database. To override a route the dispatcher stores in the Configuration only the rule:
Configuration::updateValue('PS_ROUTE_'.$route_id, $rule);

Configuration::updateValue('PS_ROUTE_'.$route_id, $rule);

This is what is stored in database:

capture d ecran 2018-08-17 a 16 01 00

So if we add a config PS_ROUTE_supplier_rule it will override the default one (not alias it), and if we add a rule with another name, say PS_ROUTE_legacy_supplier_rule, the Dispatcher won't be able to know which controller it is linked to.

{
Configuration::loadConfiguration();
$legacyRoutes = array(
'supplier_rule' => '{id}__{rewrite}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Double underscore here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I know that was the former rule.. no really clear since the manufacturer_rule is the same with one underscore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @PierreRambaud any other remarks about this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

last comment and I approve, sorry for the delay ;)

@@ -303,6 +303,8 @@ private function initContext()
if (!isset(Context::getContext()->language) || !Validate::isLoadedObject(Context::getContext()->language)) {
if ($id_lang = (int) $this->getConfValue('PS_LANG_DEFAULT')) {
Context::getContext()->language = new Language($id_lang);
} else {
Context::getContext()->language = new Language();
Copy link
Contributor

Choose a reason for hiding this comment

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

easier to read and refactor

$langId = (int) $this->getConfValue('PS_LANG_DEFAULT');
Context::getContext()->language = new Language(
    $langId ?
    $langId :
    null
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it really necessary to write on three lines?

Context::getContext()->language = new Language($langId ? $langId : null);

the one line style is readable as well no?

PierreRambaud
PierreRambaud previously approved these changes Aug 27, 2018
@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Aug 27, 2018
@ntiepresta ntiepresta added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Aug 27, 2018
@PierreRambaud
Copy link
Contributor

@jolelievre Little rebase and go for merge :)

@jolelievre
Copy link
Contributor Author

ok done :)

@jolelievre
Copy link
Contributor Author

Fixes #9509

@PierreRambaud PierreRambaud merged commit 356500e into PrestaShop:develop Aug 30, 2018
@PierreRambaud
Copy link
Contributor

Thanks @jolelievre
No need to add Fixes #TicketNumber if you edit your first comment and add it in Fixed ticket? :) (for the next time)

@jolelievre
Copy link
Contributor Author

Yep, I figured that out in some other PRs :P

@jolelievre jolelievre deleted the BOOM-6258 branch September 6, 2018 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants