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

Fixed comparing kirby version #177

Open
wants to merge 2 commits into
base: develop
from

Conversation

@afbora
Copy link
Contributor

afbora commented Nov 30, 2019

Related issue

https://forum.getkirby.com/t/kirby-editor-throws-kirby-version-error-but-version-is-correct/16502

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Fixed code style issues with CS fixer and composer fix
  • Added in-code documentation (if needed)
@distantnative

This comment has been minimized.

Copy link

distantnative commented Nov 30, 2019

If you now check for higher than 3.3.0, the error message should say as well.

But are you sure 3.3.1 is required and not 3.3.0?

@afbora

This comment has been minimized.

Copy link
Contributor Author

afbora commented Nov 30, 2019

@distantnative Hmm, also composer.json file says ^3.2 🤔

"require-dev": {
    "getkirby/cms": "^3.2"
}
@lukasbestle

This comment has been minimized.

Copy link

lukasbestle commented Nov 30, 2019

This bug was already fixed by @bastianallgeier in 04bb713 (Editor 1.0.1). Probably @plagasul used Editor 1.0.0 for testing.

Regarding the require-dev setting in composer.json: The dev requirements don't have an effect when the plugin is installed, only when the composer install is run inside the plugin's folder for development. I'd assume that Bastian set that version constraint when 3.3.0 wasn't out. In the end it doesn't matter. :)

@afbora

This comment has been minimized.

Copy link
Contributor Author

afbora commented Nov 30, 2019

@lukasbestle so editor plugin require min Kirby v3.3.1, right?

@lukasbestle

This comment has been minimized.

Copy link

lukasbestle commented Nov 30, 2019

No, 3.3.0.

@afbora

This comment has been minimized.

Copy link
Contributor Author

afbora commented Dec 1, 2019

@lukasbestle so it's should be like my PR? Exception will throw on equal values now.

So throw an exception only 3.3.0 > KIRBY mean KIRBY < 3.3.0. Am I wrong?

@afbora

This comment has been minimized.

Copy link
Contributor Author

afbora commented Dec 1, 2019

This bug was already fixed by @bastianallgeier in [04bb713]

@lukasbestle Unfortunately this issue was corrected missing. Lets compare if Kirby version is 3.3.0

if (version_compare('3.3.0', '3.3.0', '>=')) {
    throw new Exception('The editor requires Kirby version 3.3.0 or higher');
}

Exception will throw because of values are equal.
It must be at least 3.3.1 to pass the condition here.

If the minimum version supports 3.3.0, I insist that the comparison is incorrect 😄

I think the condition should be written in order to understand it more clearly:

if (version_compare($this->kirby()->version(), '3.3.0', '<')) {
    throw new Exception('The editor requires Kirby version 3.3.0 or higher');
}
@lukasbestle

This comment has been minimized.

Copy link

lukasbestle commented Dec 1, 2019

Ah, I see it now. The comparison was the wrong way round. Thanks for spotting that!

It should be:

version_compare($this->kirby()->version(), '3.3.0', '>=') !== true

Or your changed variant, which is exactly the same. :)

@lukasbestle lukasbestle reopened this Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.