Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closing API #1392

Merged
merged 2 commits into from
Dec 15, 2021
Merged

Closing API #1392

merged 2 commits into from
Dec 15, 2021

Conversation

core23
Copy link
Member

@core23 core23 commented Dec 9, 2021

Subject

I am targeting this branch, because this is BC.

Changelog

### Added
- Add `SnapshotInterface::getId()` method

### Changed
- Mark all classes as final

@core23 core23 added the minor label Dec 9, 2021
@core23 core23 requested a review from a team December 9, 2021 19:04
@core23 core23 force-pushed the final branch 2 times, most recently from 55477c8 to bb4344b Compare December 9, 2021 19:07
@core23 core23 force-pushed the final branch 3 times, most recently from bf92787 to 23dcf4c Compare December 9, 2021 19:15
Comment on lines 229 to 233
$snapshot = $this->getMockBuilder(SnapshotInterface::class)
->disableOriginalConstructor()
->addMethods(['getId'])
->getMockForAbstractClass();
// @phpstan-ignore-next-line
Copy link
Member

Choose a reason for hiding this comment

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

If we need this method, we should add it to the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not possible on the stable branch...

Copy link
Member

Choose a reason for hiding this comment

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

At least @method should be added https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Model/SnapshotInterface.php#L28
And a next major can be added in the test to update the code in the test.

But in this special case, I'm not sure we can consider that adding getId to the interface is a BC-break.
It's called here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/SnapshotManager.php#L91, so the method must be implemented anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

the phpstan ignore is not needed anymore i think

@VincentLanglet VincentLanglet requested review from jordisala1991 and a team December 9, 2021 20:07
@core23
Copy link
Member Author

core23 commented Dec 14, 2021

Ping @sonata-project/contributors

@VincentLanglet
Copy link
Member

Can you take a look @jordisala1991 ?

@@ -18,6 +18,7 @@
*
* @author Thomas Rabaix <thomas.rabaix@sonata-project.org>
*
* @method int|null getId()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be int? or mixed?

Based on PageInterface

/**
* Returns the id.
*
* @return mixed
*/
public function getId();

(I don't know if TargetId should be mixed as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

The parentId is a int type at the moment:

* @method void setParentId(?int $parentId)

If we change this, we should change it everywhere.

@VincentLanglet VincentLanglet merged commit 3a0426c into sonata-project:3.x Dec 15, 2021
@VincentLanglet
Copy link
Member

Thanks

@core23 core23 deleted the final branch December 15, 2021 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants