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

Update composer.json requirements #16714

Merged
merged 1 commit into from Jan 8, 2020

Conversation

@gavinkalika
Copy link
Contributor

gavinkalika commented Dec 5, 2019

Questions Answers
Branch? develop
Description? Updating composer.json
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Issue, please write "Fixes #[issue number]" here.
How to test? composer install

This change is Reviewable

@gavinkalika gavinkalika requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 5, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Dec 5, 2019

Hello @gavinkalika!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@gavinkalika

This comment has been minimized.

Copy link
Contributor Author

gavinkalika commented Dec 5, 2019

example of file being used TinyMceMaxLengthValidator

@gavinkalika

This comment has been minimized.

Copy link
Contributor Author

gavinkalika commented Dec 6, 2019

I am not sure if I chose the correct category

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 8, 2019

I did it for you

@gavinkalika

This comment has been minimized.

Copy link
Contributor Author

gavinkalika commented Dec 9, 2019

thanks! I do not understand the categories. Could you link a description please?

Finally, thanks for the review, but when does it get merged?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 9, 2019

https://devdocs.prestashop.com/1.7/contribute/contribution-guidelines/#category
@gavinkalika When another developer will review and merge it ;)

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 9, 2019

Could you detail a bit in the description why you needed to add this dependency?
Also, should we update the composer.lock? @PierreRambaud wdyt?

@gavinkalika

This comment has been minimized.

Copy link
Contributor Author

gavinkalika commented Dec 9, 2019

@jolelievre

No you do not need to update the composer.lock.

It is a very small change that provides consistency to the dependency list.
This is not a novel idea previous presta shop developers have done it before.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 9, 2019

My bad @jolelievre is right, the composer.lock file needs to be updated.

Copy link
Contributor

PierreRambaud left a comment

composer.lock needs to be updated

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 9, 2019

Could you add ext-json since we're at it? I get tons of warning in PHPStorm because of this

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 9, 2019

And to be really consistent we should have the same as in the doc page I guess: https://devdocs.prestashop.com/1.7/basics/installation/system-requirements/#extensions
(I can see that JSON extension is not present in the doc page by the way)

And I guess ext-gd and ext-dom (at least) are missing in the composer file

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 9, 2019

According to https://github.com/PrestaShop/php-ps-info/ we need these ones (the doc is SO outdated 😅):

Capture d’écran 2019-12-09 à 18 48 25

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 10, 2019

@jolelievre We should update php-ps-info, the composer files, and ConfigurationTest (but in a new PR)

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 10, 2019

Well php-ps-info needs to me modified in another PR of course But about composer and ConfigurationTest since they are related to the same issue why not updating them in the same PR?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 10, 2019

@jolelievre You're right, I was talking about the php-ps-info 😅

@gavinkalika gavinkalika force-pushed the gavinkalika:update-composer branch from cb02812 to 7faec9b Dec 27, 2019
gavin
@gavinkalika gavinkalika force-pushed the gavinkalika:update-composer branch from 7faec9b to 0514ef6 Dec 27, 2019
@gavinkalika

This comment has been minimized.

Copy link
Contributor Author

gavinkalika commented Dec 27, 2019

To update lock I ran

composer update --lock --ignore-platform-reqs

https://devhints.io/composer

@matks
matks approved these changes Dec 30, 2019
@matks matks dismissed PierreRambaud’s stale review Dec 30, 2019

Requested change has been applied

@matks matks changed the title update composer.json dependencies Update composer.json requirements Dec 30, 2019
@matks
matks approved these changes Dec 30, 2019
@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Jan 8, 2020
@PierreRambaud PierreRambaud merged commit 7072919 into PrestaShop:develop Jan 8, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 8, 2020

No need QA. Thanks @gavinkalika

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.