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

Added a service to manage PrestaShop versions (may deprecate _PS_VERSION_) #9174

Merged
merged 10 commits into from Jun 18, 2018

Conversation

Projects
None yet
5 participants
@michaelKaefer
Contributor

michaelKaefer commented Jun 7, 2018

Questions Answers
Branch? develop
Description? Adds functionality to retrieve the current PS version and to compare a given version to the current PS version. In this PR the PS version is hardcoded in AppKernel (which replaces PS_VERSION) - In Symfony the version is hardcoded in Kernel::VERSION. A Version object can be retrieved from the service container.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? -
How to test? If the PR is ok I will provide unit tests

This change is Reviewable

@prestonBot prestonBot added the develop label Jun 7, 2018

* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/
namespace PrestaShop\PrestaShop\Core\Foundation\PrestaShopVersion\Exception;

This comment has been minimized.

@sarjon

sarjon Jun 8, 2018

Member

no need to duplicate PrestaShop name.

- PrestaShop\PrestaShop\Core\Foundation\PrestaShopVersion\Exception
+ PrestaShop\PrestaShop\Core\Foundation\Version\Exception

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@sarjon Thank you! I wanted to make it explicit :) Anyway, I changes it and it looks better now 👍

$versionParts = explode('.', $version);
if (4 !== count($versionParts)) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 8, 2018

Contributor

Hum, you can replace both throw by a simple regex

preg_match('~^(?:(\d+)\.?){1,}\d+$~', $version);

It's invalid if it don't match 1.N, A version can be 1.0, 1.9.1, 1.2.3.4. We can go further by adding support of alpha, beta, rc, etc. Making this class a little generic could be better for the community :)

Or you can play with this regex:
[0-9]+(?>\.[0-9a-zA-Z]+)*(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?
=)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud I will change it to a regex. - RC and Beta releases have the same version hardcoded like the stable release as far as I checked it (e.g. "1.7.3.0."). - Are you sure that it's good to make this class generic? I would keep it as strict as possible, if the string is d+.d+.d+.d+ it is valid, otherwise not. Why do we need 1.0 etc? I saw something like that hardcoded to ControllerTest.php but is there a strong reason to do that? - The method is private by the way

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud I forget the most important: it is hard to accept "1.0" because version_compare() says it is less than "1.0.0". So it would be neceassary to handle all these cases seperatly which sounds bad. I changed it to a regex, please have a look. If a developer wants to compare "1.0" to the current PS version he has to transform it to "1.0.0.0" before - this does not seem too bad for me. Are you OK with this?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

Making this class working for modules can be a good idea I think, not only for "How prestashop managed their versions", wdyt @mickaelandrieu ?

throw new InvalidArgumentException($e->getMessage());
}
return version_compare(AppKernel::VERSION, $version, $operator);

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 8, 2018

Contributor

set AppKernel::VERSION as parameter in constructor, so everyone will be able to use this =)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud The method is private and not meant to be used outside the class. There are quite some other methods to compare a version string to the actual PS version. Is this ok?

@@ -26,6 +26,7 @@
namespace PrestaShopBundle\Controller\Admin\Improve\Design;
use PrestaShop\PrestaShop\Core\Foundation\PrestaShopVersion\PrestaShopVersion;

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 8, 2018

Contributor

It's maybe redundant:
PrestaShop\PrestaShop\Core\Foundation\Version

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud It's because auf PSR 4 autoloading. If there is a directory Version with a class Version this is necessary. Or do I get something wrong? By the way I changed "PrestaShopVersion" to "Version" 👍

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

I didn't find anything about PS4 autoloader and version directory :/ https://www.php-fig.org/psr/psr-4/ . Even Symfony doesn't have Version\Version, can you show me where do you see that? I think you can remove Version directory and have Version.php inside Foundation, wdyt master @mickaelandrieu ?

@@ -32,6 +32,12 @@
class AppKernel extends Kernel
{
const VERSION = '1.7.4.0';
const MAJOR_VERSION_STRING = '1.7';

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 8, 2018

Contributor

Not sure it's a good idea to register in const MAJOR, MINOR and RELEASE const, we can get this value from VERSION. We can retrieve this information from the string. Making this more generic can be useful for the community in case they also need to manage version (from dependencies for example). :)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud I did it because Symfony did it 🤣 (https://github.com/symfony/http-kernel/blob/master/Kernel.php) I don't know why they did, I just thought there must be some good reason. Should I remove it still? - What do you mean by "manage version from dependencies"?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

In this case, we can still have it in dependency injection.
If you have modules or themes for example, which need to manage some versions for anything like API versions, dependencies versions, modules versions or manage his own version, having a class to do it will be so useful for everyone. No need to rebuild the wheel :)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 10, 2018

Contributor

@PierreRambaud Thanks, now everything is injected, which really makes much more sense and looks much better! - For the other thing: if I had to decide it I would keep it as simple as possible and not make it more generic for other uses as long as it is not needed. Also, you know that PS version is very unique because it ignores semver.

*/
public function getVersion()
{
return AppKernel::VERSION;

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 8, 2018

Contributor

Must be a parameter in constructor =)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 8, 2018

Contributor

@PierreRambaud Thanks, I made a commit and it looks much better now :)

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 8, 2018

I like this PR, but as I said in comments, making this class more generic will be awesome \o/
(We like to plya with class like this in ruby https://github.com/rubygems/rubygems/blob/master/lib/rubygems/version.rb )

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 8, 2018

@PierreRambaud Thank you for your review! I will work on it 👍

throw new InvalidVersionException('A valid version must be a string.');
}
if (!preg_match('/^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$/', $version)) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

Watch out, as I said, not all versions have x.x.x.x. Can be x.x, x.x.x, and x.x.x.x (and sometimes, x.x.x-(alpha|beta|rc\d+) )

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 9, 2018

Contributor

@PierreRambaud I know, I already answered above :)

prestashop.core.foundation.version:
class: PrestaShop\PrestaShop\Core\Foundation\Version\Version
arguments:
- !php/const:AppKernel::VERSION

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 9, 2018

minor issue:

The !php/const: tag to indicate dumped PHP constants is deprecated since Symfony 3.4 and will be removed in 4.0. Use the !php/const (without the colon) tag instead on line 5 at line 5 (near "- !php/const:AppKernel::VERSION").

@@ -32,6 +32,12 @@
class AppKernel extends Kernel
{
const VERSION = '1.7.4.0';

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

I already wanted to introduce this but @eternoendless was against this change because this makes the release harder to do (not that hard, but still^^).

So before continue, we need @eternoendless approval.

@mickaelandrieu mickaelandrieu changed the title from Add Core\Foundation\PrestaShopVersion to Added Core\Foundation\Version Jun 9, 2018

@mickaelandrieu mickaelandrieu requested a review from eternoendless Jun 9, 2018

@mickaelandrieu mickaelandrieu changed the title from Added Core\Foundation\Version to Added a service to manage PrestaShop versions (may deprecate _PS_VERSION_) Jun 9, 2018

@@ -32,6 +32,7 @@
use PrestaShop\PrestaShop\Adapter\Validate;
use PrestaShopBundle\Entity\AdminFilter;
use PrestaShopBundle\Service\DataProvider\Admin\ProductInterface;
use AppKernel;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

further improvement: inject the shop version in constructor.

@@ -26,6 +26,8 @@
namespace PrestaShop\PrestaShop\Adapter\Requirement;
use AppKernel;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

same here, this should be injected in constructor.

*/
public function getMajorVersion($type = self::STRING)
public function getStringMajorVersion()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

Why 2 functions? Because I think a function shouldn't do 2 different things when we can do otherwise. More this make each function easier to test and easier to understand for your IDE (do I need a string or an integer?)

*
* @return static The created exception.
*/
public static function mustBeAString()

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

what do you think about it @michaelKaefer?

I have the feeling this make the class useful and the code easier to read in Version.

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 17, 2018

@mickaelandrieu @PierreRambaud , thank you. I made some more commits to inject it as a dependency (not everywhere but still). What will happen now? @eternoendless , can you review or approve this PR?

$sfContainer = SymfonyContainer::getInstance();
$globalParameters = ['_ps_version' => $sfContainer->get('prestashop.core.foundation.version')->getVersion()];

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 18, 2018

Contributor

should be in the if statement

$sfContainer = SymfonyContainer::getInstance();
$globalParameters = ['_ps_version' => $sfContainer->get('prestashop.core.foundation.version')->getVersion()];

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 18, 2018

Contributor

should be in the if statement in l.68

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 18, 2018

Contributor

@mickaelandrieu Thanks and done 👍

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

LGTM => this doesn't change the release process, we only have to change release version in only one place :)

@mickaelandrieu mickaelandrieu merged commit 79384e7 into PrestaShop:develop Jun 18, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 18, 2018

Thanks @michaelKaefer!

* Class responsible of managing the right version of Shop
* for every internal/external services.
*/
class Version

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

@mickaelandrieu The name is still redundant Version\Version, why version should be an exception when we don't want to do like that for other classes :/

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@PierreRambaud As I said before I think it is a necessity for PSR-4? Is that wrong? Other question: is it kind of forbidden in PrestaShop project? In Symfony this is quite common.

This comment has been minimized.

@sarjon

sarjon Jun 19, 2018

Member

maybe Versioning\Version would be better?

@@ -26,6 +26,7 @@
namespace PrestaShopBundle\Controller\Admin\Improve\Design;
use PrestaShop\PrestaShop\Core\Foundation\Version\Version;

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 19, 2018

Contributor

Useless line

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@PierreRambaud I will fix it this evening with a new PR, thank you!

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 19, 2018

@mickaelandrieu You merged it too fast I think :D

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

it is in review state for 11 days, and we - you and I - have reviewed it at least 3 times.
If it could be improved, let's do another pull request: we have until the release of 1.7.5 ;)

public function __construct($version, $majorVersionString, $majorVersion, $minorVersion, $releaseVersion)
{
$this->version = $version;

This comment has been minimized.

@sarjon

sarjon Jun 19, 2018

Member

$this->version is not defined in class as private $version;

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@sarjon Ups, I will fix it in the evening, thank you!

*/
public function getFullVersion()
{
return $this->fullVersion;

This comment has been minimized.

@sarjon

sarjon Jun 19, 2018

Member

this always returns null. You should use $this->version instead or rename $this->version to $this->fullVersion

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@sarjon Ups, I will fix it in the evening, thank you!

*
* @return bool
*
* @throws InvalidArgumentException If the provided version is invalid

This comment has been minimized.

@sarjon

sarjon Jun 19, 2018

Member

do you mean InvalidVersionException instead of InvalidArgumentException?

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@sarjon Ups, I will fix it in the evening, thank you!

private $releaseVersion;
public function __construct($version, $majorVersionString, $majorVersion, $minorVersion, $releaseVersion)

This comment has been minimized.

@sarjon

sarjon Jun 19, 2018

Member

wouldnt it be better to pass single full version like '1.7.4.0' and then parse different formats instead of passing a lot of versions? @mickaelandrieu what do you think?

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 19, 2018

Contributor

@sarjon @PierreRambaud said the same above. I just did it like Symfony does it without knowing the reason. Anyway I like it like it is and Symfony will have some goog reason. If you change it in another PR it's also ok for me of course.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

this always returns null. You should use $this->version instead or rename $this->version to $this->fullVersion

$this->version is not defined in class as private $version;

do you mean InvalidVersionException instead of InvalidArgumentException?

@sarjon would you mind to do a pull request to do the updates?

wouldnt it be better to pass single full version like '1.7.4.0' and then parse different formats instead of passing a lot of versions? @mickaelandrieu what do you think?

I'm not sure yet, but if you think this is better, add it to your pull request :)

maybe Versioning\Version would be better?

I'm not really fan of Versioning, how about Version\ShopVersion instead ?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

@michaelKaefer you need to make a new pull request as this one is merged, thanks for your help 👍

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 19, 2018

I'm not really fan of Versioning, how about Version\ShopVersion instead ?

As I said, we maybe can make this class generic in case we use this for modules, src/Core/Foundation/Version.php, it can be a base about how PrestaShop use versions for core, modules, etc.

Ok to make it in Core/Foundation directly

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 19, 2018

@PierreRambaud As I said I don't think this is a good idea. The class is for PS versions only because nearly nobody else in the PHP world (modules, packages, etc.) will use the format with 4 numbers, only PS does.

And I think we need a directory - or where can we put the exception class?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 19, 2018

And I think we need a directory - or where can we put the exception class?

in src/Core/Foundation/Exception? ping @PierreRambaud we need a decision here before michael starts to work on it please :)

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 19, 2018

@mickaelandrieu @PierreRambaud I really don't see why Version/Version is bad. It's common to use redundant names in other well coded projects and I don't consider it as bad practise at all. And in my opinion it's much better than ShopVersion and similar inventions..

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 19, 2018

@sarjon @PierreRambaud @mickaelandrieu I fixed everything in a new PR (except from the Version\Version thing).

@michaelKaefer michaelKaefer deleted the michaelKaefer:core-foundation-version branch Jun 19, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 20, 2018

Yes I know, but for Symfony, it's because class is in Form tool, Version isn't a tool, it always depends in which context the class is. Same for Monolog, if you see, this class is never used directly, it's an abstract class (and there's also AbstractHandler.php, it's weird).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment