Use lorenzo/pinky, breaks compatibility with PHP < 5.6

[tchapi]

Following https://github.com/gremo/ZurbInkBundle/issues/1 and @solverat's code :

• We use https://github.com/lorenzo/pinky instead of hampe/inky
• This breaks compatibility with PHP < 5.6.0

[gremo]

Hey, thanks for this PR.

Have a look at https://github.com/gremo/ZurbInkBundle/blob/master/src/Resources/config/services.xml service Pinky should be created and injected into HtmlUtils (if possible).

[tchapi]

Oh, I noticed that I forgot to remove the old Inky service from the class, I will change that. As for injecting Pinky, since it's not a class but a namespace, I don't think this is possible or even a good practice, or I am missing something ?

[gremo]

Well about the injection, i seriously don't know. Let me think about it.

What about the PHP version bump in composer? Maybe my understandings of composer/semver are low, but this project still require PHP > 5.3 and not PHP 5.6... am I right? That is, we are not directly using features from PHP 5.6, it's only Pinky requiring that version.

[tchapi]

For reference : https://github.com/lorenzo/pinky/blob/master/src/pinky.php I don't think you can 'inject' that..

As for the version bump, I followed Pinky's composer.json blindly. There must be a reason that it's PHP 5.6 there. If not, then yes, we could totally lower the requirement — though I'm not sure if composer would choke when installing if you're on 5.3 and it finds a requirement for 5.6 in a sub-vendor composer.json ...

[gremo]

For the version bump, I don't think we should consider all the dependencies listed in composer.json to figure out the minimum PHP version requirements, am I wrong? @cierzniak @davidromani what do you think?

I think composer is smart to figure it out, letting you to install the latest version of this package (not including pinky) if you are running PHP < 5.6.

[tchapi]

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for lorenzo/pinky 1.0.1 -> satisfiable by lorenzo/pinky[1.0.1].
    - lorenzo/pinky 1.0.1 requires php >=5.6.0 -> your PHP version (7.2.7) overridden by "config.platform.php" version (5.3.2) does not satisfy that requirement.

I'm afraid we need the version bump. It gives a better indication on what is the minimum required for the bundle (instead of finding out when composer install ing).

[tchapi]

composer will find the previous release (that will still have 5.3.2) and install it, but if we don't bump this one, this will lead to an uninstallable set of packages as per my previous comment I think. Happy to have other inputs on this too !

[gremo]

I think the problem here is the config.platform.php composer setting.

[tchapi]

This is just a setting to emulate running 5.3.2 instead of my actual PHP that is 7.2, but it's equivalent to running 5.3.2 from a composer point of view

[gremo]

@tchapi yes I know.. but isn't the right behavior? You are faking 5.3.2 and try to install this package. Should install 3.0.1 assuming the requirement is "^3.0.0" It should work.

When we bump the version (say) to 3.1.0, the new requirement for Pinky should be considered and composer should stick with 3.0.1.

[tchapi]

This is my understanding of the composer resolver (but I clearly might be wrong):

• Version 3.0.1 requires PHP > 5.3.2
• Let's say we create Version 3.1.0 that requires the same PHP > 5.3.2 as you propose

Then when composer will try to find the adequate package for some guy (on PHP 5.3.2) installing the library, it will look at these releases and will find that both require a minimum of 5.3.2, so it will pick the latest one, which will fail.

That is to say, when choosing the release, composer only looks at the root composer.json requirements.

If we bump :

• Version 3.0.1 requires PHP > 5.3.2

• Version 3.1.0 requires PHP > 5.6.0

  When composer installing (the same guy with the same PHP version of 5.3.2), composer will discard the 3.1.0 release since it's not compatible and will successfully install 3.0.1.

Again, this is my understanding that composer will not resolve all dependencies of all packages (and their dependencies, and their dependencies, etc) before choosing which version to install.

@damienalexandre long time no talk but I think you're the perfect expert to enlighten us on the subject ;)

[gremo]

@tchapi I see.... good to know. Can we easly test this beahviour with your fork? Maybe you can push a new tag 3.1.0 and see what happens...

[tchapi]

https://github.com/tchapi/ZurbInkBundle/releases/tag/v3.1.0

[GenieTim]

Why is the break with PHP version so bad? PHP versions < 5.6 are not supported PHP versions anyway. Of course, this does not mean that this bundle should not support PHP < 5.6. Currently, it does, which is good, of course. If you simply merge this PR with a major version bump (4.0.0) after creating a branch v3 from the current state as a legacy branch supporting PHP < 5.6, you will be able to support both platforms, PHP < 5.6 as well as PHP > 7.1. Git does not force you to create tags in increasing order, this way you will be able to, if you need, release updates to the PHP < 5.6 supporting version too, by tagging changes to the v3 branch like 3.y.x. Composer will decide, if you use PHP < 5.6, you will have v3 installed, otherwise v4. Or, people can manually specify in composer.json to use version ^v3.0. And everyone is happy. Or am I missing the point here?

[gremo]

@BernhardWebstudio I agree with you and that's the way we should follow.

I'm only concerned about the following: raising the PHP version requirements to 5.6 even if it's not true in the sense that the code I wrote actually works with PHP 5.3, is this right?

I've no much time at the moment, can you confirm what @tchapi is saying about the PHP version requirement in composer.json?

[GenieTim]

Yes, I confirm that the change in minimum PHP version in this PR is for this repositorys benefit. It does not actually matter, but makes it easier (and presumably faster) for composer to determine the correct versions of dependencies.

[ChoppyThing]

Hello, is this pr soon accepted ? Thanks

[gremo]

@ChoppyThing done! Cheers!

[ChoppyThing]

Thanks a lit @gremo ! :champagne:

[tchapi]

Thanks a lot for merging and tagging, @gremo !