search

codefog / contao-haste

Haste is a collection of tools and classes to ease working with Contao
http://codefog.pl/extension/haste.html
MIT License
42 stars 24 forks source link

Changed \String::* calls to \StringUtil for PHP7 compatibility reasons #79

Closed bytehead closed 8 years ago

bytehead commented 8 years ago

PHP 7 compatibility - see contao/core-bundle#309.

aschempp commented 8 years ago

I think StringUtil has been introduced in Contao 3.5.2 ?

bytehead commented 8 years ago

Hmm, that could be. But full PHP7-compat was introduced with 3.5.5 afaik.

bytehead commented 8 years ago

Actually 3.5.1...

aschempp commented 8 years ago

3.5.5, true: https://github.com/contao/core/issues/8018

Toflar commented 8 years ago

Imho we can also increase the minimum compatibility to 3.5.5. All the other extensions that use the String class and want to be compatible with PHP7 are very likely to be at least 3.5.5 instead of using version_compare() anyway too? What are your opinions? Time to move on, maybe?

bytehead commented 8 years ago

Moving on sounds great for me. I don't know, how many others use this extension. By the way, it's a bc break, isn't it?

aschempp commented 8 years ago

It's not a BC break because the Haste API does not change. Only the dependencies change, and Composer will take care of that.

However, I'm pretty sure people still use Haste in older versions (likely we do, too), so… dunno…

Toflar commented 8 years ago

No BC break but yes, you cannot update to that version anymore then without moving on. But in that case you are using a long outdated Contao version that probably needs to be updated anyway. Is it just this piece of code for PHP7 compat?

bytehead commented 8 years ago

I think so. I'm not sure about the func_get_args() here - as Julien Pauli from SensioLabs states a behavioural change in his talk (direct link to Slide 55). The assignment ($dc = $arg;) foreach (func_get_args() as $arg) looks not affected to me. Your opinion?

Toflar commented 8 years ago

Should not, no :) Thx for the link btw :) Well then, we would increase the dependencies just for that one piece of code where we could easily live with a version_compare, right? Then I'd just leave it as is.

bytehead commented 8 years ago

No worries :) I can easily live with a version_compare. We could could mark it to be removed in a next major version.

bytehead commented 8 years ago

Update version with correct version compare (inclusive BUILD number). See comment from @discordier.

fritzmg commented 8 years ago

But shouldn't it still be compared with version 3.5.1 technically, as this particular change only regards the String class usage, which is available in Contao 3.5.1?

qzminski commented 8 years ago

@aschempp @Toflar so what do we finally do with this issue? Shall I merge this PR or do you have any extra feedback?

@fritzmg perhaps it could be compared to 3.5.1 as well (not sure about the Andy's comment though https://github.com/codefog/contao-haste/pull/79#issuecomment-182368680) but what I'd recommend more is to update your installation to 3.5.11 anyway as no BC break is there compared to 3.5.1 ;-)

fritzmg commented 8 years ago

@fritzmg perhaps it could be compared to 3.5.1 as well (not sure about the Andy's comment though #79 (comment)) but what I'd recommend more is to update your installation to 3.5.11 anyway as no BC break is there compared to 3.5.1 ;-)

I know, but technically Contao\StringUtil is available since Contao 3.5.1 so imho that's the version it should be compared to - but it doesn't matter in the end anyway :)

qzminski commented 8 years ago

Merged in dcda68c.