Fix BC break with DoctrineType::reset() for all Symfony versions

[javer]

Fixes BC break with mistakenly removed kernel.reset tag from form.type.entity service, because DoctrineType implements ResetInterface only in Symfony 4.2+, therefore all previous versions starting from 3.4 are affected.

This BC break was introduced in version 1.11.0 in commit 6b25ea4

[alcaeus]

See feedback on the conditional.

[javer]

@alcaeus Fixed, please check.

[xabbuh]

This checks the version of the FrameworkBundle which doesn't have to be the same as the one of the DoctrineBridge (which provides the type).

[alcaeus]

So we also need to check whether the type implements the ResetInterface?

[javer]

@alcaeus @xabbuh What do you think about this solution?

[alcaeus]

If this works reliably (I’m not sure because I don’t know when autoconfig is applied, ping @nicolas-grekas) I don’t believe we need the other checks at all 🤔

[javer]

@alcaeus It seems to be the most reliable solution according to above comments, because method reset could be removed from DoctrineType in the future versions of DoctrineBridge.

[alcaeus]

Sorry, more feedback needed from @doctrine/team-symfony-integration.

[alcaeus]

@alcaeus It seems to be the most reliable solution according to above comments, because method reset could be removed from DoctrineType in the future versions of DoctrineBridge.

That would be a BC break which would only be possible in a major release: this bundle would not install such a version without modifications.

[javer]

@alcaeus So we return to the first solution, which was correct, because it:

• checks whether symfony/form component is installed
• checks whether DoctrineType class has method reset
• adds kernel.reset tag to the service regardless it's already added or not

[javer]

@alcaeus @xabbuh Any thoughts?

[alcaeus]

The safe choice:

• check whether symfony/form is installed. If not, skip the rest and don't do anything.
• check whether ResetInterface exists. If not, skip the rest of the checks and add the tag.
• check whether DoctrineType implements ResetInterface. If not, skip the rest of the checks and add the tag.
• check whether AbstractController has an addLink method. If not, add the tag.

Note that checking for the reset method is unnecessary - the minimum version we require has that. It's the ResetInterface that's only available in newer versions and is also tied to autoconfiguration.

The "yeah, this won't break choice":

• Just add the tag.

As I said - I couldn't find any side effects from having two kernel.reset tags in there, so we may as well just add the tag. I'm also not sure if the tag would even be added a second time by the autoconfiguration mechanism.

[javer]

@alcaeus Done, please check.

[javer]

@alcaeus @xabbuh Hi guys, I have tested this fix with Symfony 4.2 and 4.3 and have just realized that even in these recent versions of Symfony kernel.reset tag isn't added automatically, because automatic tagging works only for autoconfigured services: https://github.com/symfony/symfony/blob/4.3/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php#L50
But this service is not marked as autoconfigured, even when you make all your services in the app autoconfigurable, because autoconfigure option works only for services defined in the same yml file: https://symfony.com/doc/current/service_container.html#the-autoconfigure-option

Actually this ResetInterface haven't ever worked since adding it by @nicolas-grekas in commit symfony/symfony@8982036#diff-0b86d71e1a20e98ac050dd33c4e79373R29

So I modified code to explicitly add kernel.reset tag always if we have symfony/form component installed. All other checks aren't necessary and even wrong.

[xabbuh]

The check for the existence of the method is still necessary, isn't it? Otherwise the kernel might call try to call the method because of the tag while it actually doesn't exist.

[javer]

@xabbuh Well, actually this check is not so critical, because removing reset method from DoctrineType will be a BC break in Symfony Doctrine Bridge, but I added this check for safety.

By the way, we returned to the implementation I already did 16 days ago: #970 (comment)

[xabbuh]

Ah sorry, that's my mistake. I thought the method wasn't present in Symfony 3.4.

[xabbuh]

So if we don't need the check, can't we just add the tag in the XML config file where we define the service config?

[javer]

No, unfortunately we cannot hardcode this tag, because this bundle doesn't have symfony/form as a dependency, so we should add this tag only when application uses this component.

I have already tried to do this unconditionally, and unit tests were failed.

[alcaeus]

Sorry for all the back and forth here - I guess we were all blind. However, we can just check if symfony/form is installed, everything else is (as you already mentioned) unnecessary. I'll tag a new release once the changes below have been made.

[alcaeus]

Thanks @javer, and sorry for taking a few laps around the block to realise what's going on here!