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

Make PS compatible with PHP 7.2 #8246

Merged
merged 18 commits into from Apr 23, 2018

Conversation

Projects
None yet
7 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Aug 11, 2017

Questions Answers
Branch? develop
Description? When testing PrestaShop with PHP 7.2, many errors appeared in the PHP error log. This PR begins the work to do with this version.
Type? new feature
Category? CO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? http://forge.prestashop.com/browse/BOOM-4444
How to test? Test on a server running with PHP 7.2.

Related PRs:


This change is Reviewable

@Quetzacoalt91 Quetzacoalt91 added the WIP label Aug 11, 2017

Show outdated Hide outdated .travis.yml Outdated
@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Nov 20, 2017

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

Issues
======
+ Solved 2
- Added 14
           

Complexity increasing per file
==============================
- classes/Tools.php  2
- classes/Currency.php  1
- tools/profiling/Controller.php  1
- src/PrestaShopBundle/Api/QueryParamsCollection.php  1
         

Complexity decreasing per file
==============================
+ classes/controller/AdminController.php  -1
         

See the complete overview on Codacy

codacy-bot commented Nov 20, 2017

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

Issues
======
+ Solved 2
- Added 14
           

Complexity increasing per file
==============================
- classes/Tools.php  2
- classes/Currency.php  1
- tools/profiling/Controller.php  1
- src/PrestaShopBundle/Api/QueryParamsCollection.php  1
         

Complexity decreasing per file
==============================
+ classes/controller/AdminController.php  -1
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Nov 20, 2017

@Quetzacoalt91 Quetzacoalt91 removed the WIP label Nov 20, 2017

@Quetzacoalt91 Quetzacoalt91 referenced this pull request Dec 5, 2017

Merged

Update toCamelCase #8584

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 9, 2018

Member

Reviewed 26 of 30 files at r1, 13 of 13 files at r2.
Review status: 35 of 39 files reviewed at latest revision, 3 unresolved discussions.


classes/controller/AdminController.php, line 566 at r2 (raw file):

     * @param int|null $tab_id
     */
    public function initBreadcrumbs($tab_id = null)

We shouldn't change the signature of public methods until the next major version


classes/controller/AdminController.php, line 572 at r2 (raw file):

        }

        $tabs = Tab::recursiveTab($tab_id, array());

Why remove the $tab parameter?


classes/controller/AdminController.php, line 3512 at r2 (raw file):

        }

        if (is_null($filter_modules_list) || !count($filter_modules_list)) {

Shouldn't this be !is_array($filter_modules_list) || !count(...)?


Comments from Reviewable

Member

eternoendless commented Jan 9, 2018

Reviewed 26 of 30 files at r1, 13 of 13 files at r2.
Review status: 35 of 39 files reviewed at latest revision, 3 unresolved discussions.


classes/controller/AdminController.php, line 566 at r2 (raw file):

     * @param int|null $tab_id
     */
    public function initBreadcrumbs($tab_id = null)

We shouldn't change the signature of public methods until the next major version


classes/controller/AdminController.php, line 572 at r2 (raw file):

        }

        $tabs = Tab::recursiveTab($tab_id, array());

Why remove the $tab parameter?


classes/controller/AdminController.php, line 3512 at r2 (raw file):

        }

        if (is_null($filter_modules_list) || !count($filter_modules_list)) {

Shouldn't this be !is_array($filter_modules_list) || !count(...)?


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Jan 12, 2018

Member

classes/controller/AdminController.php, line 3512 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Shouldn't this be !is_array($filter_modules_list) || !count(...)?

$filter_modules_list can be an array or null. That's an opportunity to remind this (tests were broken without that change).


Comments from Reviewable

Member

Quetzacoalt91 commented Jan 12, 2018

classes/controller/AdminController.php, line 3512 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Shouldn't this be !is_array($filter_modules_list) || !count(...)?

$filter_modules_list can be an array or null. That's an opportunity to remind this (tests were broken without that change).


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Jan 12, 2018

Member

Review status: 34 of 39 files reviewed at latest revision, 3 unresolved discussions.


classes/controller/AdminController.php, line 566 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

We shouldn't change the signature of public methods until the next major version

You're totally right. Reverted


Comments from Reviewable

Member

Quetzacoalt91 commented Jan 12, 2018

Review status: 34 of 39 files reviewed at latest revision, 3 unresolved discussions.


classes/controller/AdminController.php, line 566 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

We shouldn't change the signature of public methods until the next major version

You're totally right. Reverted


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Jan 12, 2018

Member

Comments taken in account. Do not forget to check the related module PR, needed to make this one working on Travis.

Member

Quetzacoalt91 commented Jan 12, 2018

Comments taken in account. Do not forget to check the related module PR, needed to make this one working on Travis.

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Feb 14, 2018

Contributor

:lgtm:


Reviewed 4 of 4 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

Contributor

LittleBigDev commented Feb 14, 2018

:lgtm:


Reviewed 4 of 4 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Feb 15, 2018

Member

:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


classes/controller/AdminController.php, line 3512 at r2 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

$filter_modules_list can be an array or null. That's an opportunity to remind this (tests were broken without that change).

Ok then


Comments from Reviewable

Member

eternoendless commented Feb 15, 2018

:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


classes/controller/AdminController.php, line 3512 at r2 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

$filter_modules_list can be an array or null. That's an opportunity to remind this (tests were broken without that change).

Ok then


Comments from Reviewable

@eternoendless eternoendless added this to the 1.7.4.0 milestone Feb 15, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Feb 15, 2018

Member

After a closer look, it looks like AdminControllerCore::setMedia() has a different signature than ControllerCore::setMedia(), that will probably need fixing too


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Member

eternoendless commented Feb 15, 2018

After a closer look, it looks like AdminControllerCore::setMedia() has a different signature than ControllerCore::setMedia(), that will probably need fixing too


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Build is still failing

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Feb 19, 2018

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Feb 19, 2018

Member

It seems not having the same signature as an abstract class is not an issue, fortunately. Otherwise we'd have to update all the front controllers, although they do not need this variable.

Tested with

abstract class Controller
{
    abstract public function setMedia();
}

class AdminController extends Controller
{
    public function setMedia($isNewTheme = false)
    {
        echo "AdminController\n";
    }
}

class FrontController extends Controller
{
    public function setMedia()
    {
        echo "FrontController\n";
    }
}

class MyController extends AdminController
{
    public function setMedia($isNewTheme = false)
    {
        echo "MyController\n";
    }
}

(new AdminController)->setMedia();
(new FrontController)->setMedia();
(new MyController)->setMedia();
Member

Quetzacoalt91 commented Feb 19, 2018

It seems not having the same signature as an abstract class is not an issue, fortunately. Otherwise we'd have to update all the front controllers, although they do not need this variable.

Tested with

abstract class Controller
{
    abstract public function setMedia();
}

class AdminController extends Controller
{
    public function setMedia($isNewTheme = false)
    {
        echo "AdminController\n";
    }
}

class FrontController extends Controller
{
    public function setMedia()
    {
        echo "FrontController\n";
    }
}

class MyController extends AdminController
{
    public function setMedia($isNewTheme = false)
    {
        echo "MyController\n";
    }
}

(new AdminController)->setMedia();
(new FrontController)->setMedia();
(new MyController)->setMedia();

@Quetzacoalt91 Quetzacoalt91 dismissed stale reviews from LittleBigDev and eternoendless via f0c6a04 Apr 19, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Apr 23, 2018

Member

Reviewed 3 of 3 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Apr 23, 2018

Reviewed 3 of 3 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Apr 23, 2018

Thank you @Quetzacoalt91

@eternoendless eternoendless merged commit e678517 into PrestaShop:develop Apr 23, 2018

2 checks passed

code-review/reviewable 38 files 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