Narrow API

[phansys]

Subject

Narrow API.

Following Rector sets are used:

• ParamTypeDeclarationRector;
• ReturnTypeDeclarationRector;

I am targeting this branch, because these changes belong to the next major.

See #5613 (comment).

Changelog

### Added
- Missing argument and return type declarations.
### Fixed
- Inconsistencies with wrong types passed to some methods.

To do

• Remove Rector dependency and configuration;
• Fix all the remaining inconsistencies;

[phansys]

Currently, the work in this PR is scoped at Sonata\AdminBundle\Admin\ namespace (the rest of the changes are required by changes originally made here).

Based on this PoC I think we should make some decisions, by instance:

• Should FieldDescriptionInterface::getTemplate() disallow null as return value? see here;
• Should AdminInterface::id(), AdminInterface::getNormalizedIdentifier(), etc. disallow other types than string for its return values? see here.

I'd like to use this PR as base to find the better solution for each case.
Also, should be great if someone can use this work as base and fix the remaining failed tests (at least the more obvious).

Thank you in advance.

[phansys]

Just for the record, I'm leaving here the test results with the current state:

Tests: 1434, Assertions: 3956, Errors: 16, Failures: 5, Warnings: 2, Skipped: 1.

Remaining direct deprecation notices (2)

  1x: The "Symfony\Component\Validator\Constraint::__sleep()" method is considered internal. It may change without further notice. You should not extend it from "Sonata\Form\Validator\Constraints\InlineConstraint".
    1x in AdminTest::testFormAddPostSubmitEventForPreValidation from Sonata\AdminBundle\Tests\Admin

  1x: The "Sonata\AdminBundle\Translator\Extractor\JMSTranslatorBundle\AdminExtractor" class implements "Symfony\Component\Translation\TranslatorInterface" that is deprecated since Symfony 4.2, use Symfony\Contracts\Translation\TranslatorInterface instead.
    1x in AdminExtractorTest::setUp from Sonata\AdminBundle\Tests\Translator\Extractor\JMSTranslatorBundle

Legacy deprecation notices (14)

[VincentLanglet]

Is it a good idea to add this to the 4.0 major ?
I mean, the 4.0 is already a big release, waited for years and introducing multiple breaking changes.

Adding param/return typehint will add a lot of breaking changes.
Shouldn't the param/return typehint wait for the 5.0, even if it's mainly the only change of the major ?

Btw: Since there was no @return void in the Phpdoc, it seems like the : void return type is not added by the rector.

[greg0ire]

Since we require php 7.2, adding param type declarations is no longer a BC-break from a signature compatibility point of view, it's only a BC break for people not strictly obeying the phpdoc when calling our APIs, or in cases where our phpdoc is not accurate. So it's not a big BC-break. I would add a least the param type declarations (could even be done on 3.x if we were super confident in our phpdoc, not sure we are.)

[phansys]

Is it a good idea to add this to the 4.0 major ?

I think we can close the API as much as possible for 4.x release.

Since there was no @return void in the Phpdoc, it seems like the : void return type is not added by the rector.

I guess the missing declarations could be updated later, even in other PRs.
Maybe we should even relax some declarations in cases where we don't know the full context yet (mean, : ?string instead of : string by instance).

could even be done on 3.x

IMO, we can't because in the majority of these cases, the added declarations are part of the public API, so the projects using these classes will break if these declarations are not updated in these projects too.
BTW, based on the light brought by these changes, I was opening separate PRs for each case I think can be merged before (#6011, #6017).

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[phansys]

Other example. Should we really keep this behavior?

$this->listMapper->add('fooNameLabelFalse', null, ['label' => false]);
$this->listMapper->get('fooNameLabelFalse')->getOption('label'); // false

IMO, if possible, is preferable to return values like value|null instead of value|bool, since it allows us to use ?type declarations for arguments and return types.

WDYT?

[VincentLanglet]

Other example. Should we really keep this behavior?

$this->listMapper->add('fooNameLabelFalse', null, ['label' => false]);
$this->listMapper->get('fooNameLabelFalse')->getOption('label'); // false

IMO, if possible, is preferable to return values like value|null instead of value|bool, since it allows us to use ?type declarations for arguments and return types.

WDYT?

In your exemple, the value label is set to false, so I expect to get false.

[phansys]

The example is based on this test:

SonataAdminBundle/tests/Datagrid/ListMapperTest.php

Lines 177 to 189 in dcaef0b

	$this->listMapper->add('fooNameLabelFalse', null, ['label' => false]);
	$this->assertTrue($this->listMapper->has('fooName'));
	$fieldDescription = $this->listMapper->get('fooName');
	$fieldLabelBar = $this->listMapper->get('fooNameLabelBar');
	$fieldLabelFalse = $this->listMapper->get('fooNameLabelFalse');
	$this->assertInstanceOf(FieldDescriptionInterface::class, $fieldDescription);
	$this->assertSame('fooName', $fieldDescription->getName());
	$this->assertSame('fooName', $fieldDescription->getOption('label'));
	$this->assertSame('Foo Bar', $fieldLabelBar->getOption('label'));
	$this->assertFalse($fieldLabelFalse->getOption('label'));

I agree that an option which is not restricted by an options resolver can hold anything, but in this case, the value of this option is being used as return type of a method (getLabel()) which is declared in a interface (FieldDescriptionInterface):

SonataAdminBundle/src/Datagrid/ListMapper.php

Line 109 in dcaef0b

if (null === $fieldDescription->getLabel()) {

SonataAdminBundle/src/Admin/BaseFieldDescription.php

Lines 372 to 375 in dcaef0b

	public function getLabel()
	{
	return $this->getOption('label');
	}

SonataAdminBundle/src/Admin/FieldDescriptionInterface.php

Lines 239 to 244 in dcaef0b

	/**
	* return the label to use for the current field.
	*
	* @return string
	*/
	public function getLabel();

If we can define the return type of FieldDescriptionInterface::getLabel() as ?string, maybe we should change the implementation at BaseFieldDescription::getLabel() to something like this:

public function getLabel(): ?string
{
    return null === $this->getOption('label') ? null : (string) $this->getOption('label'); 
} 

Otherwise, we must keep the FieldDescriptionInterface::getLabel() signature open to return any type.

[VincentLanglet]

Otherwise, we must keep the FieldDescriptionInterface::getLabel() signature open to return any type.

IMO we shouldn't modify the logic/code just in order to add return type. If we have to wait for union type, we'll wait.

getOption should return the exact value set. If we set 'label' => false, I expect to get false, the same value that I should get when I called $options['label'] in a FormType.

getLabel seems more like a helper with no similar function in Symfony. So I would have nothing against returning ?string. But I expect the return value null if getOption('label') return false, not '' like your example.

[phansys]

That's exactly the point @VincentLanglet. I agree with your proposal of returning null if false is submitted. But that's only a implementation detail.
I'm concerned here about the API closing. In other words, what values will we declare and allow in our interfaces.
Based on the feedback we could collect here, we should decide the way to go forward.

[phansys]

@sonata-project/contributors, I'd like to achieve some consensus on this before performing more changes.

[greg0ire]

I'm concerned here about the API closing. In other words, what values will we declare and allow in our interfaces.

IMO we should avoid mixed (or union types) as much as possible. Let's have a code that is easy to understand for both people and SA tools.

[phansys]

IMO we should avoid mixed (or union) as much as possible. Let's have a code that is easy to understand for both people and SA tools.

That's exactly what I'm trying to avoid here 👍
IMO, after merging this, we must following the fixing task for the remaining cases in other PRs.

[phansys]

Based on this concern (#6005 (comment)), I've updated all the methods intended to return model ids in order to allow only string|null as return type, given all the current implementations are returning strings:

• SonataDoctrineORMAdminBundle;
• SonataDoctrineMongoDBAdminBundle;
• SonataDoctrinePhpcrAdminBundle.

I guess this PR is RTM.
/cc @sonata-project/contributors.

[VincentLanglet]

I'm trying to make changes from simple to complex here. Even, this PR is becoming unmanageable for me given the amount of changes.
That's why I'd like to merge ASAP in order to follow this work in more atomic PRs later.
That way, I think we could start to catch early more API violations coming from 3.x.

My concern is that if we merge a half-typed method, or half-typed class, it'll be easy to miss it when we'll release the next major. And then it will be impossible to add the type before the 5.0 version.

I agree that this PR is big and not easy to review, it's still possible to make a soft reset and create smaller PR now, file by file.

[phansys]

My concern is that if we merge a half-typed method, or half-typed class, it'll be easy to miss it when we'll release the next major. And then it will be impossible to add the type before the 5.0 version.

I agree that this PR is big and not easy to review, it's still possible to make a soft reset and create smaller PR now, file by file.

Indeed. I'm not suggesting to merge a half-typed method, but try to focus this step in the methods that are already updated.
The methods which I've left in the majority of cases are methods that pass their arguments to classes coming from other namespaces or even other packages. That's why I'd prefer to work on them later, since we need to decide if these methods should be updated too.
BTW, thank you so much for the review 👍 There were various missing type declarations between some interfaces and their implementation.

[VincentLanglet]

I think this is the last changes needed if you don't want to touch the methods with $entity arguments of the AbstractAdmin.

[VincentLanglet]

I found other missing return type.
I promise, next time I'll approve.

[phansys]

I found other missing return type.
I promise, next time I'll approve.

IIRC, I didn't touch the extension part by hand. Those changes was applied automatically by Rector.

[phansys]

Or maybe you prefer to add something like a NEXT_MAJOR: Decide if we support string or string|array ? In the same way we could add NEXT_MAJOR: Decide if we support object or ?object for the method of AbstractAdmin.

I've added some "NEXT_MAJOR" comments addressing these situations.

I agree there is controversial decision. What is the best way to be sure to take a decision about them ?

I think the better option will be to adopt a wider perspective among the other namespaces when all the possible non-controversial narrowing was done.

[VincentLanglet]

Sounds perfect. Nice work @phansys

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

Let's try to merge this ! @sonata-project/contributors

[greg0ire]

Thanks a lot @phansys !