common icon indicating copy to clipboard operation
common copied to clipboard

The future of Common 3.0

Open Majkl578 opened this issue 7 years ago • 38 comments

The proposal is pretty simple: have no Common 3.0, instead split Common into smaller packages. We've already had discussion about this and Common 3.0 seems pretty much like relic and multiple components have already been split to their own packages.

Current proposal:

Component Suggested action
Persistence Move to new package doctrine/persistence (rename namespace to Doctrine\Persistence)
Proxy To be dropped without replacement.
Reflection Not really sure about this one, either move to doctrine/reflection or drop it.
Util/ClassUtils Proxy-related methods are obsolete making the rest also pretty much obsolete - drop it
Util/Debug Drop it or move to doctrine/debug.
Util/Dumper Unsure about its usefulness (there already exist more advanced tools like Tracy or Symfony Dumper).
Util/Inflector Already moved out, only BC.
ClassLoader Already deprecated, drop it.
CommonException ~
Comparable I know no place where Comparable is used across Doctrine (is there any?) but it seems useful i.e. for ORM 3.0 - move it to doctrine/comparable.
EventArgs + EventManager + EventSubscriber ~We've already discussed replacing it by i.e. Symfony's EventDispatcher - drop it if suitable replacement is selected,~ doctrine/event-manager.
Lexer Already moved out, only BC.
NotifyPropertyChanged + PropertyChangedListener These sound useful and are used in ORM so ~move it to doctrine/property-change-notifier (what a name!)~ doctrine/persistence.
Version ~

Each new package would use PHP 7.2 (or even 7.1 where 7.2 has no benefit), follow CS and would be ported to typed code.

WDYT?

Majkl578 avatar Dec 06 '17 04:12 Majkl578

EventArgs + EventManager + EventSubscriber We've already discussed replacing it by i.e. Symfony's EventDispatcher - drop it if suitable replacement is selected, doctrine/event-manager.

Only if proven to be faster at runtime.

Lexer Already moved out, only BC.

Deprecate and use Hoa components

NotifyPropertyChanged + PropertyChangedListener These sound useful and are used in ORM so move it to doctrine/property-change-notifier (what a name!).

Move to ORM, rework as isolated layer if possible

Version ~

Drop it - relying on package versions should only be done via composer.json

Ocramius avatar Dec 06 '17 06:12 Ocramius

NotifyPropertyChanged + PropertyChangedListener These sound useful and are used in ORM so move it to doctrine/property-change-notifier

Move to ORM, rework as isolated layer if possible

Would ODM need these as well?

jmikola avatar Dec 06 '17 15:12 jmikola

NotifyPropertyChanged + PropertyChangedListener These sound useful and are used in ORM so move it to doctrine/property-change-notifier

Move to ORM, rework as isolated layer if possible

Would ODM need these as well?

We'd need to port these as well

malarzm avatar Dec 06 '17 15:12 malarzm

I know no place where Comparable is used across Doctrine (is there any?) but it seems useful i.e. for ORM 3.0 - move it to doctrine/comparable.

We were thinking of using it to compare value objects instead of comparing them by reference in ODM 2.0. Extracting it to a separate package would make sense.

alcaeus avatar Dec 07 '17 06:12 alcaeus

The proposal is pretty simple: have no Common 3.0, instead split Common into smaller packages.

Yes, please. doctrine/common should die. I've given the whole BC problem and the fact that upgrading one to 3.0 will conflict with a bunch of other stuff, so here's an idea on how to migrate to 3.0 versions of everything:

  1. Split code in individual packages, remove from doctrine/common
  2. update composer.json to require all these new packages (e.g. doctrine/comparable)
  3. Add autoload file to doctrine/common that triggers a deprecation notice for doctrine/common itself
  4. Release the whole contraption as 2.9.0: this essentially makes doctrine/common a metapackage that throws a deprecation notice, but keeps everything else intact
  5. Start updating our own dependents of doctrine/common to no longer require doctrine/common but individual packages
  6. Start releasing 3.0 versions with BC breaks of those individual packages

Reflection: Not really sure about this one, either move to doctrine/reflection or drop it.

What do we need it for? IIRC there are far better reflection libraries out there, so we might want to use those

Each new package would use PHP 7.2 (or even 7.1 where 7.2 has no benefit), follow CS and would be ported to typed code.

Ack on PHP versions, Ack on CS, not entirely sure about typed code. Many instances probably are fine, but there are some cases where we might not want to add type hints just yet. Take the following interface for example:

interface Collection
{
    // ...
    public function get(int $index): object;
}

Makes sense, right? Except that without full co- and contravariance in PHP, I can't do this:

class SpecialCollection implements Collection
{
    // ...
    public function get(int $index): MyEntityClass {}
}

This is perfectly fine when Collection::get does not specify a return type, but fails when it does. I believe there are a few cases where we might want to hold back on strictly typing our interfaces to allow for stricter implementations.

alcaeus avatar Dec 07 '17 18:12 alcaeus

We were thinking of using it to compare value objects instead of comparing them by reference in ODM 2.0. Extracting it to a separate package would make sense.

Same idea is there for ORM 3.0 to compare DBAL values and Embeddables by value.

Add autoload file to doctrine/common that triggers a deprecation notice for doctrine/common itself

Also class aliases will be needed since we change namespace.

Start releasing 3.0 versions with BC breaks of those individual packages

For consistency with other packages that were already split earlier, we should start versioning at 1.0.

Reflection

What do we need it for?

I have found no reference for it. It was added as part of #154/#152 probably as a more memory effective variant based on PSR-0.

not entirely sure about typed code

Collection as it is designed now is all about mixed members, not object, so no object typehints there unless we decide it will be object-only. Your example is otherwise valid, there is no variance along with object due to controversy and possible future scope. But honestly I know noone (or no project) that'd use custom collections to change return types (and you couldn't change parameter for i.e. Collection::contains(object $element) types anyway). In general for such case (same applies more or less to EntityManager or ORM repositories), object is the only correct way and if you miss the opportunity to add it now, you can miss it for another X years while possibly having PHP variance for object natively (or not) in future 7.x release, who knows.

Majkl578 avatar Dec 07 '17 19:12 Majkl578

Also class aliases will be needed since we change namespace.

We haven't done so for inflector, lexer, collections, or other packages, which is why I didn't think of it.

and you couldn't change parameter [...] types anyway

You're right - this one is probably best solved using documentation (or generics in PHP)

alcaeus avatar Dec 08 '17 06:12 alcaeus

We haven't done so for ...

Hmm true, they are still in Doctrine\Common namespace... That should probably also change in major release of those packages + providing forward compat by adding class aliases in 1.x (something like PHPUnit did).

Majkl578 avatar Dec 09 '17 04:12 Majkl578

That should probably also change in major release of those packages + providing forward compat by adding class aliases in 1.x

FWIW, this is something that has been discussed for annotations 2.0, where it has triggered quite the discussion: https://github.com/doctrine/annotations/pull/75#discussion_r56872787. I'm split on this issue: on one hand, cleaning up namespaces would be nice, on the other hand it would make things way more complicated. I will however concede that we could ease the migration by providing a forward compat layer. That would allow people to better prepare for the new package versions.

alcaeus avatar Dec 15 '17 13:12 alcaeus

When it comes to changing package's namespace, I think most people are concerned about compatibility between both major versions - x.y & x+1.0. This shouldn't be hard to achieve with mentioned forward compatibility layer where we introduce new namespace and class aliases in x.y version and migrate classes in x+1.0. Something like PHPUnit did around version 4. Then doing such migration is as easy as one regexp replace (sed/IDE) over whole code base.

Majkl578 avatar Dec 17 '17 01:12 Majkl578

Util/Debug Drop it or move to doctrine/debug.

I suggest dropping it. this Debug utility is shitty anyway, as dumping an uninitialized PersistentCollection will have the side effect of initializing it (and so dumping some variable to debug something will change the behavior of the code, which could then not have the bug you were debugging). And the output format is not nice either. We should just recommend symfony/var-dumper instead.

stof avatar Mar 07 '18 14:03 stof

Update:

  • Event Manager was separated into doctrine/event-manager: #842
  • Persistence was separated into doctrine/persistence: #844
  • Reflection was separated into doctrine/reflection: #844

Currently with undecided fate (so still left without deprecation):

  • NotifyPropertyChanged + PropertyChangedListener
  • Comparable

Deprecations for the rest of remaining things via #845, targeting 2.9.0 which should be released in upcoming days.

Majkl578 avatar Jun 19 '18 23:06 Majkl578

https://www.doctrine-project.org/2018/07/12/common-2-9-and-dbal-2-8-and-orm-2-6-2.html

Majkl578 avatar Jul 12 '18 22:07 Majkl578

Great job, happy to see so much progress towards 3.0! 🎉

pamil avatar Jul 13 '18 09:07 pamil

Is there a checklist available to describe the steps to be taken?

Obviously remove doctrine/common....

Which Doctrine packages should be installed to cover the removal of doctrine/common?

Am I correct in presuming:

  • doctrine/persistence
  • doctrine/event-manager
  • doctrine/reflection

?

sterichards avatar Oct 02 '18 15:10 sterichards

@sterichards doctrine/common would provide the list inside composer.json (it already does for some moved out packages)

Ocramius avatar Oct 02 '18 15:10 Ocramius

@Ocramius In that case, it would be:

  • "doctrine/inflector"
  • "doctrine/cache"
  • "doctrine/collections"
  • "doctrine/lexer"
  • "doctrine/annotations" ~~- "doctrine/event-manager"~~ ~~- "doctrine/reflection"~~ ~~- "doctrine/persistence"~~

sterichards avatar Oct 02 '18 15:10 sterichards

Except the last three, the other were moved out years ago. Please see the release notes for 2.9.0 where the recent changes are described.

Majkl578 avatar Oct 02 '18 15:10 Majkl578

Removing doctrine/common is looking more and more difficult as nearly every latest stable doctrine package requires doctrine/common

As an example - https://packagist.org/packages/doctrine/data-fixtures#v1.3.1:

Requires:

  • doctrine/common: ~2.2

sterichards avatar Oct 02 '18 16:10 sterichards

@sterichards we'd just migrate each of our own downstream package: we're not breaking BC anyway, so relying on doctrine/common is fine for now

Ocramius avatar Oct 02 '18 16:10 Ocramius

Removing doctrine/common is looking more and more difficult

The ultimate aim is not to drop doctrine/common in all consumers now, but to phase it out gradually. First major step was separating components that will outlive Common. Right now DBAL already benefits from this change (see https://github.com/doctrine/dbal/pull/3181). It's more tricky in other pacakges - i.e. ORM uses ProxyGenerator and NotifyPropertyChanged so it can't drop doctrine/common in 2.x, only ORM 3.0.

TL;DR: In long-term it's preferable if can make your application depend on specific packages instead of doctrine/common (your dependencies may still draw doctrine/common in, but that wouldn't be your problem anymore - new packages are co-installable with doctrine/common 2.9+). Otherwise doctrine/common is still here and not abandoned, and it won't (can't) be at least until ORM 3.0.

Majkl578 avatar Oct 02 '18 16:10 Majkl578

Makes sense

I was looking for an approach that would eliminate deprecated warnings, which is what lead me to this issue ticket initially

sterichards avatar Oct 02 '18 19:10 sterichards

Makes sense

I was looking for an approach that would eliminate deprecated warnings, which is what lead me to this issue ticket initially

Agree, please remove the deprecation warning since it's quite annoying and it's a false positive, it's not something the developer of a Symfony app is supposed to fix

fmonts avatar Oct 02 '18 19:10 fmonts

Send a patch: https://www.doctrine-project.org/policies/deprecation.html

Ocramius avatar Oct 02 '18 19:10 Ocramius

I was looking for an approach that would eliminate deprecated warnings

please remove the deprecation warning since it's quite annoying and it's a false positive

Which warning(s) exactly? Most people notice the issue coming from ORM (https://github.com/doctrine/doctrine2/issues/7306 that will be fixed in ORM 2.6.3 once released.

Majkl578 avatar Oct 02 '18 20:10 Majkl578

I see the deprecated warning when running PHPUnit

It's no hassle to me if it's going to be resolved in a future version and isn't causing any lack of functionality at the moment

sterichards avatar Oct 02 '18 20:10 sterichards

If anybody else comes across this thread, there is a method for suppressing deprecation notices in PHPUnit - https://stackoverflow.com/questions/35897550/remove-remaining-deprecation-notices-232-in-symfony-2-8

sterichards avatar Oct 04 '18 15:10 sterichards

I was looking for an approach that would eliminate deprecated warnings

please remove the deprecation warning since it's quite annoying and it's a false positive

Which warning(s) exactly? Most people notice the issue coming from ORM (doctrine/doctrine2#7306 that will be fixed in ORM 2.6.3 once released.

Why it hasn't been released yet? How long will it take to have a release? It's been many months now...

fmonts avatar Nov 21 '18 16:11 fmonts

lol, it was released yesterday :)

good job, the deprecation warning is finally gone

fmonts avatar Nov 21 '18 16:11 fmonts

What about Doctrine\Common\Util\ClassUtils::getRealClass(string $class) ? Where is that function going to live? Or what is the replacement?

TerjeBr avatar Mar 13 '19 14:03 TerjeBr