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

Synchronise about:version command to last Prestashop version. #250

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

MeKeyCool
Copy link
Contributor

Take a look at PrestaShop/PrestaShop#27632, ModuleRepositoryInterface as been moved

@nenes25
Copy link
Member

nenes25 commented May 9, 2022

Hi @MeKeyCool,

Thanks for your PR 😄 the fix you suggest is related with the improvements done in Prestashop 8 i guess ?

As we want to keep this tool compatible with not only the lastest version of Prestashop, i think we should use a way where we can manage both v8 and other version.

ping @SebSept @jf-viguier WDYT ?

@nenes25 nenes25 requested review from SebSept and jf-viguier May 9, 2022 16:31
@MeKeyCool
Copy link
Contributor Author

Hi @MeKeyCool,

Thanks for your PR smile the fix you suggest is related with the improvements done in Prestashop 8 i guess ?

As we want to keep this tool compatible with not only the lastest version of Prestashop, i think we should use a way where we can manage both v8 and other version.

ping @SebSept @jf-viguier WDYT ?

Yes, this change is related to a breaking change in Prestashop v8.
On my develop branch, I can't run fop:about:version command.

🤔 As it is only a check, I think I can make something that work for both version ...

I'll try tomorrow ;)

@SebSept
Copy link
Contributor

SebSept commented May 10, 2022

At the moment, we havn't decide to change the minimal support version.
We must start to think about it (open a discussion can be good).

I have a plan to make a command that displays information about a module, I guess I'll write a custom dataprovider for the purpose. So we can use that dataprovider for the current command.

@SebSept
Copy link
Contributor

SebSept commented May 10, 2022

For the moment, we can can use something more simple that will support both v8 and 1.7.5

By the way, can't we use services ? It will be easy to check if a service is defined or not.

Copy link
Contributor

@SebSept SebSept left a comment

Choose a reason for hiding this comment

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

At the moment support for v8 and 1.7.5 is needed.

@MeKeyCool MeKeyCool requested a review from SebSept May 19, 2022 08:33
@MeKeyCool
Copy link
Contributor Author

@SebSept , was your change request about the type check ?

@SebSept
Copy link
Contributor

SebSept commented Jun 2, 2022

@SebSept , was your change request about the type check ?
I can't remember. No matter.
I works fines in my tests.
Thanks for your patience.

@SebSept SebSept requested review from nenes25, a user and tom-combet June 2, 2022 19:45
Copy link
Contributor

@tom-combet tom-combet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 😃👍
Little change requested but not mandatory since you check the type later.

src/Commands/About/AboutVersion.php Show resolved Hide resolved
@SebSept SebSept added the bug Something isn't working label Jun 3, 2022
@SebSept SebSept merged commit 73e57c9 into friends-of-presta:dev Jun 3, 2022
@SebSept
Copy link
Contributor

SebSept commented Jun 3, 2022

Thanks @MeKeyCool 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants