[Next Major] Close API

[core23]

• [ ] Add final modifier
• [ ] Reduce visiblity to private
• [ ] Add type hints

[greg0ire]

Now that 7.1 is EOL (https://secure.php.net/supported-versions.php), maybe we should bump to 7.2 add parameter type declarations in a new minor? If we consider current docblocks as a contract?

[core23]

👎 For adding parameter types. There is a reason why symfony won't do this in a major release...

👍 For adding return types

[greg0ire]

👍 for adding return types to final classes, but not otherwise, and not to interfaces either: https://3v4l.org/KLvG9

:-1: For adding parameter types. There is a reason why symfony won't do this in a major release...

You mean a minor release, and what's the reason?

[core23]

👍 for adding return types to final classes, but not otherwise, and not to interfaces either: https://3v4l.org/KLvG9

👍 for full return / parameter types to final classes

You mean a minor release, and what's the reason?

If we add a parameter type and someone uses an other type hint, it would break: https://3v4l.org/XBfik

[greg0ire]

But… they can't use a type declaration if we don't, that would break the LSP, and php rightfully forbids that: https://3v4l.org/eiigE

[greg0ire]

Also, we can add type declarations to private methods.

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[VincentLanglet]

As explained here some interface are required/useful if we want to add final modifier to some class.

• PoolInterface
• MapperInterface
• GroupMapperInterface
• DatagridMapperInterface
• ListMapperInterface
• FormMapperInterface
• ShowMapperInterface
• AdminObjectAclDataInterface
• AdminObjectAclManipulatorInterface

And maybe SearchHandlerInterface.

[VincentLanglet]

I saw you add final to lot of classes @franmomu https://github.com/sonata-project/SonataAdminBundle/pull/6819 The last one is AdminHelper. Declaring this class as final had complexity to our unit tests since we can't mock it anymore. What is your opinion about this class ?

AdminHelper::getChildFormView and getChildFormBuilder seems better in another service IMHO

The AdminHelper::appendFormFieldElement is only used by AppendFormFieldElementAction maybe an service is not worth it and we should prefer move the method from the AdminHelper to the AppendFormFIeldElementAction.

[vv12131415]

since we can't mock it anymore

you can do https://github.com/dg/bypass-finals

[franmomu]

I haven't checked that class too much, I'll try (when I can) to see if I can instantiate it, another solution could be to make it @internal as looks like internal to us. It is used as dependency in AppendFormFieldElementAction and RetrieveFormFieldElementAction, but IMO these actions are meant to be called through request, not manually instantiating them.

[VincentLanglet]

another solution could be to make it @internal as looks like internal to us. It is used as dependency in AppendFormFieldElementAction and RetrieveFormFieldElementAction, but IMO these actions are meant to be called through request, not manually instantiating them.

I agree. I thought about this too.

[dmaicher]

I had a look and its indeed not simple to use a "real" instance of AdminHelper in the tests. Involves fiddling around with FormFactory, FormBuilder, ...

I vote for making it @internal

[dmaicher]

Is there anything else to do for this issue?

The AdminHelper class is @internal now. See https://github.com/sonata-project/SonataAdminBundle/pull/6842

[VincentLanglet]

I think there is some minor changes possible:

• AdminHelper::$propertyAccessor can be private
• AdminHelper::getModelClassName() too

But that's all I see. Maybe more method could be final (in the AbstractAdmin or the CRUDController for instance)

[VincentLanglet]

@dmaicher I started https://github.com/sonata-project/SonataAdminBundle/pull/6848

We can add final to a lot of method.