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

Return Types for ITitledObject::getLink() and ILinkableObject::getLink() #5157

Merged
merged 4 commits into from Feb 1, 2023

Conversation

mutec
Copy link
Contributor

@mutec mutec commented Dec 2, 2022

This prevents issues in case one would return null or other types.

@TimWolla TimWolla self-requested a review December 2, 2022 13:41
@TimWolla TimWolla added this to Needs Triage in WoltLab Suite 6.0 via automation Dec 2, 2022
@TimWolla
Copy link
Member

TimWolla commented Dec 5, 2022

Can you please further split this into 2 commits each:

  1. Apply the return type to the implementing classes, this is valid, because return types are covariant.
  2. Apply the return type to the interface itself.

@mutec
Copy link
Contributor Author

mutec commented Dec 5, 2022

@TimWolla I just splitted the commits. Is there any deep sense behind that separation?

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thank you. Can you drop the annotated PHPDoc line and slightly reword the commit messages when you are rebasing anyway? “Return Value Type” misleadingly parses as “Return (Value Type)” instead of “(Value Value) Type” for me.

Let me suggest the following prasing:

  • Add proper string return type to implementations of Foo::bar()
  • Add proper string return type to Foo::bar()

wcfsetup/install/files/lib/data/article/Article.class.php Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member

TimWolla commented Dec 6, 2022

Is there any deep sense behind that separation?

Generally it's considered good commit hygiene to make commits as small and self-contained as possible. In this specific case it's logically two different types of change and by splitting them into different commits, they can be independently reverted if the need arises. If it turns out that the break from the explicit type in the interface is too bad, that one can be reverted without losing the explicit types in the implementing classes.

@TimWolla TimWolla moved this from Needs Triage to In Progress in WoltLab Suite 6.0 Dec 6, 2022
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Perfect, thank you. I'll handle updating the return types in the other packages (including the open sourced ones).

@TimWolla TimWolla merged commit a87fd58 into WoltLab:master Feb 1, 2023
WoltLab Suite 6.0 automation moved this from In Progress to Resolved Feb 1, 2023
TimWolla added a commit to WoltLab/com.woltlab.wcf.conversation that referenced this pull request Feb 1, 2023
@mutec mutec deleted the tltype branch February 1, 2023 20:08
TimWolla added a commit that referenced this pull request Feb 2, 2023
…:getLink()

These are required since #5157, because the actual event classes implement both
this interface and the ITitledObject/ILinkableObject interfaces, requiring both
interfaces to be compatible.

This breaks the `null` return value for `getLink()`, but implementing classes
can just implement the compatibility link to NotificationListPage themselves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
WoltLab Suite 6.0
  
Resolved
Development

Successfully merging this pull request may close these issues.

None yet

3 participants