[WIP] Added a build for PHP 7.4 #18190
Conversation
|
True, I've updated the PR description. Sadly, for some reason Composer fails to install the dependencies on the build. |
|
I think this pull request is useless right now. |
|
@mickaelandrieu Please modify the Travis to install the required extensions for PHP 7.4 - see why the install is failing: https://travis-ci.com/github/PrestaShop/PrestaShop/jobs/299332277 @PierreRambaud Yes, very few basic/core issues need to be fixed. But failing is otherwise ok to me as it adds value for anyone who is contributing by letting them know if changes improved the code for future PHP version or not. |
|
Hi, I can improve the pull request but I need to know if there is a chance to be merged. Everyone's time is precious, I don't want to waste mine on something that will stay stalled for months and closed in the end. I'm able to fix the "PHP extensions" part of the issue, I'm less easy spending my time fighting with my old mates on what should be done for a first pull request to support PHP 7.4. So ... let me know when your decision is done ^o^ |
|
Hi @mickaelandrieu and thank you for your contribution. I think this would make more sense to do at the same time as the modifications needed for that test to pass. Just adding a failing test (that we already know won't pass) doesn't add much value IMO, and will eat up Travis resources for nothing. If you feel motivated to start working on the PHP 7.4 compatibility, please by all means do so. If you don't feel like it or if you don't have the time, then I think it's better to close this one. Up to you mate 🙂 |
|
@mickaelandrieu The extensions are not available as very old Ubuntu dist was used - my PR #18214 fixes it, please rebase this PR on it. I would say the right time to merge this PR once the PrestaShop installation will be passing and the unit tests can be evaluated. |
|
How can I obtain a write/maintainer access to PrestaShop so I can participate on PRs like this? |
In order to be able to add commits to any contribution branch you must become a maintainer. So far in history it never happened someone that was not an employee of PrestaShop company could become maintainer but we aim to change this in 2020. However an easier way is to make a Pull Request from your fork to the contributor fork. Last week, @PululuK contributed to one of my PRs by opening a Pull Request that targeted my own fork instead of PrestaShop repository. Then you become contributor of the author fork and the author can review and merge your contribution to his own branch. |
🧐 |
src/Adapter/EntityMapper.php
Outdated
| @@ -82,7 +82,7 @@ public function load($id, $id_lang, $entity, $entity_defs, $id_shop, $should_cac | |||
| if ($object_datas_lang = Db::getInstance()->executeS($sql)) { | |||
| foreach ($object_datas_lang as $row) { | |||
| foreach ($row as $key => $value) { | |||
| if ($key != $entity_defs['primary'] && array_key_exists($key, $entity)) { | |||
| if ($key != $entity_defs['primary'] && isset($entity, $key)) { | |||
There was a problem hiding this comment.
Won't work. isset will disable null value, and we sometimes want null values
There was a problem hiding this comment.
What you want is probably property_exists()
There was a problem hiding this comment.
property_exists function bugs with protected properties sadly.
src/Adapter/EntityMapper.php
Outdated
| @@ -95,8 +95,7 @@ public function load($id, $id_lang, $entity, $entity_defs, $id_shop, $should_cac | |||
| } | |||
| $entity->id = (int) $id; | |||
| foreach ($object_datas as $key => $value) { | |||
| if (array_key_exists($key, $entity_defs['fields']) | |||
| || array_key_exists($key, $entity)) { | |||
| if (isset($entity_defs['fields'][$key]) || isset($entity, $key)) { | |||
There was a problem hiding this comment.
same here, we want null values
There was a problem hiding this comment.
Yeah yeah, I introduce back my uses of property_exists and ... time to go.
WIP label added.
mickaelandrieu
changed the title
|
I will never have enough time to focus on such big contributions sadly :/ Closed |
This change is
Got this result on my computer:
(base) ➜ PrestaShop git:(feature/php-74) composer install --no-suggest --ansi --prefer-dist
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.
Package operations: 195 installs, 0 updates, 0 removals
Package container-interop/container-interop is abandoned, you should avoid using it. Use psr/container instead.
Generating autoload files
(base) ➜ PrestaShop git:(feature/php-74) php -v
PHP 7.4.3 (cli) (built: Feb 23 2020 07:24:28) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
with Zend OPcache v7.4.3, Copyright (c), by Zend Technologies