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

Bump min compatibility to PHP 7.2.5 and PS 1.7.7, module version to 4.0.0 #941

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

leemyongpakvn
Copy link
Contributor

@leemyongpakvn leemyongpakvn commented Nov 7, 2023

Questions Answers
Description? Please check the related issue.
This PR is waiting for other PRs with major functioning changes before Release 4.0.0
Type? refacto
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#32841
How to test? CI green.
All code changes are pure format changes so there is no need to check modules functions.
The only concern is bumping doctrine/collections: from ^1.4 to ^1.8 that can affects src/Adapter/AbstractAdapter and MySQL, so a full functioning check for PS 1.7.7+ is still needed ;)

@leemyongpakvn leemyongpakvn marked this pull request as draft November 7, 2023 06:54
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

Still a few issues with phpstan

@leemyongpakvn
Copy link
Contributor Author

In fact, phpstan checks failed at composer install step with phar-io and phpunit version conflict. We are not in rush with this PR while waiting for Hlavtox's major functioning changes :)

@leemyongpakvn leemyongpakvn changed the title Bump min compatibility to PHP 7.1.3 and PS 1.7.7, module version to 4.0.0 Bump min compatibility to PHP 7.2.5 and PS 1.7.7, module version to 4.0.0 Nov 8, 2023
@leemyongpakvn leemyongpakvn force-pushed the BumpMinCompatToPHP713nPS177_moduleVersionTo400 branch from 5eb3096 to d4887a3 Compare November 8, 2023 05:29
@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Nov 8, 2023

phpstan checks failed at composer install step with phar-io and phpunit version conflict

problem is solved by dropping PHP 7.1 support, and jumping to PHP 7.2.5 minimum instead.
Keeping PHP 7.1 support is unrealistic because even PHP 7.2 reached EOL since 1 Jan 2021.
I think merging this PR will make a good base for Module Release 4.0.0 in 2024.

@leemyongpakvn leemyongpakvn marked this pull request as ready for review November 8, 2023 05:52
@leemyongpakvn leemyongpakvn marked this pull request as draft November 14, 2023 15:24
@leemyongpakvn leemyongpakvn force-pushed the BumpMinCompatToPHP713nPS177_moduleVersionTo400 branch 2 times, most recently from ab8a0cb to 9586cf9 Compare November 22, 2023 03:58
@leemyongpakvn leemyongpakvn marked this pull request as ready for review November 22, 2023 04:00
@leemyongpakvn leemyongpakvn added 4.0.0 and removed 4.0.0 labels Nov 22, 2023
@leemyongpakvn leemyongpakvn added this to the 4.0.0 milestone Nov 22, 2023
tleon
tleon previously approved these changes Nov 22, 2023
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @leemyongpakvn ,

The switch buttons are not well displayed :
Screenshot 2023-11-23 at 16 54 17

Could you check ?
Thanks!

@florine2623 florine2623 added waiting for author Waiting for author's feedback and removed Waiting for QA labels Nov 23, 2023
@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Nov 24, 2023

@florine2623 I don't see that issue on both PS1.7.8.9 and PS 8.1.x with installation made by these 2 commands:

git clone https://github.com/PrestaShop/ps_facetedsearch.git
gh pr checkout 941

Maybe you need to apply BO > Clear (PS) cache and FO > Clear (browser) cache before testing again.

@Hlavtox
Copy link
Contributor

Hlavtox commented Nov 24, 2023

@florine2623 Normal on 1.7.6, you can ignore it. 👍

@leemyongpakvn leemyongpakvn added Waiting for QA and removed waiting for author Waiting for author's feedback labels Nov 28, 2023
Copy link

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @leemyongpakvn

Thank you for your PR, I tested it and it seems to works with the version bump as you can see :

Untitled_.Dec.4.2023.3_47.PM.webm

Currently I tried it on 1.7.7.8 and with php 7.2.34 ( yes, I didn't have 7.2.5)
And I've this message when I want to have your PR :
image

As you can see most of them need 7.3 and other need 7.4 that not really 7.2.5

Another screenshot with php v7.3.33 :

image

except that, all seems to works as expected with php v7.4 and more
Tested on 1.7.7.8/8.0.x/8.1.x/develop

Waiting for your feedback

@AureRita AureRita added the waiting for author Waiting for author's feedback label Dec 4, 2023
@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 4, 2023

I am not sure, but did you run composer install and not composer update? 🤔

@leemyongpakvn leemyongpakvn force-pushed the BumpMinCompatToPHP713nPS177_moduleVersionTo400 branch from 02603cf to dbd3032 Compare December 5, 2023 04:41
@leemyongpakvn leemyongpakvn force-pushed the BumpMinCompatToPHP713nPS177_moduleVersionTo400 branch from ec7fe85 to 4cb8254 Compare December 5, 2023 07:54
@leemyongpakvn
Copy link
Contributor Author

@AureRita I see. mockery 1.6 requires php ^7.3, downgrade to mockery 1.3 solved the issue. If compose install still fails, you can try composer update as @Hlavtox suggested.
@tleon In need of reapprove after rebase ;)

@leemyongpakvn leemyongpakvn removed the waiting for author Waiting for author's feedback label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To be tested
Development

Successfully merging this pull request may close these issues.

Bump minimum compatibility to PHP 7.1.3 and PS 1.7.7 for Native Modules
7 participants