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

Remove useless call to addRoute for multilanguages shops #8209

Merged
merged 5 commits into from Oct 19, 2017

Conversation

Projects
None yet
7 participants
@jocel1
Contributor

jocel1 commented Aug 1, 2017

Questions Answers
Branch? develop
Description? Performance : remove useless call to addRoute for multilanguages shops
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? verify urls are still working properly

This change is Reviewable

@jocel1

This comment has been minimized.

Show comment
Hide comment

@aleeks aleeks changed the base branch from 1.7.2.x to develop Aug 2, 2017

@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Aug 2, 2017

Contributor

Hello @jocel1
It's not really a bug fix so i move this PR on developbranch, you need to rebase from develop!
Thank you!

Contributor

aleeks commented Aug 2, 2017

Hello @jocel1
It's not really a bug fix so i move this PR on developbranch, you need to rebase from develop!
Thank you!

@eternoendless eternoendless modified the milestones: 1.7.2.3, 1.7.3.0 Sep 18, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 3, 2017

Member

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


classes/Dispatcher.php, line 557 at r1 (raw file):

     *
     * @return array
     */

Would you mind adding a description for this method and all of its parameters?


classes/Dispatcher.php, line 564 at r1 (raw file):

            $transform_keywords = array();
            preg_match_all('#\\\{(([^{}]*)\\\:)?(' .
                implode('|', array_keys($keywords)) . ')(\\\:([^{}]*))?\\\}#', $regexp, $m);

Could you please break this line so we have one argument per line?


classes/Dispatcher.php, line 584 at r1 (raw file):

                    $regexp = str_replace($m[0][$i], $prepend_regexp .
                        '(?P<' . $keywords[$keyword]['param'] . '>' . $keywords[$keyword]['regexp'] . ')' .
                        $append_regexp, $regexp);

Here too, one argument per line please


classes/Dispatcher.php, line 588 at r1 (raw file):

                    $regexp = str_replace($m[0][$i], $prepend_regexp .
                        '(' . $keywords[$keyword]['regexp'] . ')' .
                        $append_regexp, $regexp);

Also here


classes/Dispatcher.php, line 616 at r1 (raw file):
Per PSR-2:

Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.

So please put one argument per line :)


Comments from Reviewable

Member

eternoendless commented Oct 3, 2017

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


classes/Dispatcher.php, line 557 at r1 (raw file):

     *
     * @return array
     */

Would you mind adding a description for this method and all of its parameters?


classes/Dispatcher.php, line 564 at r1 (raw file):

            $transform_keywords = array();
            preg_match_all('#\\\{(([^{}]*)\\\:)?(' .
                implode('|', array_keys($keywords)) . ')(\\\:([^{}]*))?\\\}#', $regexp, $m);

Could you please break this line so we have one argument per line?


classes/Dispatcher.php, line 584 at r1 (raw file):

                    $regexp = str_replace($m[0][$i], $prepend_regexp .
                        '(?P<' . $keywords[$keyword]['param'] . '>' . $keywords[$keyword]['regexp'] . ')' .
                        $append_regexp, $regexp);

Here too, one argument per line please


classes/Dispatcher.php, line 588 at r1 (raw file):

                    $regexp = str_replace($m[0][$i], $prepend_regexp .
                        '(' . $keywords[$keyword]['regexp'] . ')' .
                        $append_regexp, $regexp);

Also here


classes/Dispatcher.php, line 616 at r1 (raw file):
Per PSR-2:

Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.

So please put one argument per line :)


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

Show outdated Hide outdated classes/Dispatcher.php Outdated
Show outdated Hide outdated classes/Dispatcher.php Outdated
Show outdated Hide outdated classes/Dispatcher.php Outdated

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 16, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 16, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 17, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 17, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 4
- Added 3
           

Complexity increasing per file
==============================
- classes/Dispatcher.php  2
         

See the complete overview on Codacy

codacy-bot commented Oct 17, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 4
- Added 3
           

Complexity increasing per file
==============================
- classes/Dispatcher.php  2
         

See the complete overview on Codacy

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 17, 2017

Member

:lgtm:


Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 17, 2017

:lgtm:


Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Oct 17, 2017

Contributor

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Contributor

LittleBigDev commented Oct 17, 2017

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Oct 19, 2017

Thank you @jocel1

@eternoendless eternoendless merged commit 66e48e3 into PrestaShop:develop Oct 19, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 1 file reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment