Skip to content
This repository has been archived by the owner on Oct 29, 2020. It is now read-only.

Modernization - PHP 7.1, strict types, Build Stages, dropped legacy adapters etc. #122

Closed
wants to merge 11 commits into from

Conversation

Majkl578
Copy link

@Majkl578 Majkl578 commented Oct 22, 2017

This PR includes bunch of changes, some of them BC break-ish:

  • PHP 7.1 is now required, HHVM is dropped - matches other Doctrine packages
  • tested against PHP 7.2
  • removed Coveralls, obsoleted in other packages - should probably be replaced by Scrutinizer, but I don't have permissions to set that up (anyone?)
  • migrated to ext/mongodb - ext/mongo is PHP 5 only
  • dropped Riak cache support - not compatible with PHP 7 - not a real BC break
  • dropped Memcache cache support - not compatible with PHP 7 - not a real BC break
  • upgraded PHPUnit
  • replaced external Coding Standard with Doctrine's, but not applied/enforced as it'd be a lot of changes - could be done if no objections about conflicts are raised
  • enabled strict types - possible BC break, nothing revealed by tests though
  • removed licence headers - matches other Doctrine packages

Future scope:

  • PHPStan
  • apply and enforce Doctrine CS

Test failure under 7.2 looks like a redis extension bug. :/

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about those "real BC breaks" - can you explain how they are not real? Is it because riak and memcache aren't compatible with PHP 7 so technically there wouldn't be anyone running those on PHP 7?

Furthermore, if you're already doing a modernization of this scale, it might be worth switching to ::class constants and short array syntax at the same time ;)

@@ -186,13 +171,11 @@ public function getConfigTreeBuilder()
->append($this->addCouchbaseNode())
->append($this->addChainNode())
->append($this->addMemcachedNode())
->append($this->addMemcacheNode())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this invalidate existing config with a memcache key?

*
* @author Fabio B. Silva <fabio.bat.silva@gmail.com>
*/
class MemcacheDefinition extends CacheDefinition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping a class is most definitely a BC break.

@lcobucci
Copy link
Member

lcobucci commented Oct 29, 2017

Although I REALLY like the idea of modernising all our packages I think we shouldn't do it on the bundles/modules (I actually believe that we should transfer them to SF/ZF organisations, but that's a different story).

What I mean is that we did that kind of work on another bundle and SF team is reverting some changes to ensure compatibility with SF and I would like to avoid that kind of thing.

@alcaeus
Copy link
Member

alcaeus commented Oct 30, 2017

@lcobucci good point - let’s ask @fabpot how he wants to proceed.

@alcaeus
Copy link
Member

alcaeus commented Nov 28, 2017

Ping @fabpot @weaverryan is it ok to drop PHP 5 for cache-bundle or would you rather keep it (for now)?

@fabpot
Copy link
Member

fabpot commented Nov 28, 2017

I'm fine with dropping PHP 5 as long as there is still a maintained version for bug fixed on 1.3 which would keep PHP 5 compat.

@alcaeus
Copy link
Member

alcaeus commented Nov 8, 2018

Closing here - this will be introduced in multiple Mrs, starting with dropping support for PHP < 7.1 in #147.

@alcaeus alcaeus closed this Nov 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants