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

Fix mismatched parameter names in child classes (Trac 51553 - task 1) #612

Closed

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 17, 2020

This PR fixes all known instances of parameter name mismatches between methods in child classes vs the names used in the parent class.

Notes:

  • Whenever the parameter name which was originally used in the method signature of the child class was significantly more descriptive than the parameter name used in the method signature of the parent class, the parameter name in the method signature has been changed, but an assignment to the "old" parameter name is made near the top of the function to keep the readability of the rest of the function code intact. This also reduces code churn.
  • While doing these renames I noticed that a lot of these functions violate the LISKOV principle by having covariant parameters, while parameters are supposed to be contravariant. This is outside the scope of this ticket, but is a concern.
    Ref: https://www.php.net/manual/en/language.oop5.variance.php

Trac ticket: https://core.trac.wordpress.org/ticket/51553


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch from 409b3e4 to 35bbe54 Compare October 17, 2020 16:19
@jrfnl
Copy link
Member Author

jrfnl commented Oct 17, 2020

Rebased for merge conflicts due to 5b6a20a

@jrfnl
Copy link
Member Author

jrfnl commented Nov 13, 2020

@desrosj I would have happily rebased ...

@desrosj
Copy link
Contributor

desrosj commented Nov 13, 2020

Sorry! I was actively reviewing so figured I'd just update to use the same PR. Updating the ticket soon!

@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch from 34f9683 to b400b9f Compare November 23, 2020 03:02
@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch 2 times, most recently from 795f67a to f6f903f Compare June 18, 2021 17:16
@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch from f6f903f to 90955ba Compare July 9, 2021 14:16
@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch from 90955ba to e0ffece Compare July 9, 2021 17:25
jrfnl added 17 commits July 9, 2021 19:41
… as param

When the name used in the child method was significantly more descriptive, the parameter is renamed, but the old name is still used within the function itself.

Related to 51553 - Task 1 + Task 2-2
The parent class used `$current_object_id`, while most child classes used `$id`. Again, a parameter name mismatch.
As `$current_object_id` is **more** descriptive than the extremely generic `$id`, we decided to use the parent class name everywhere.

Related to 51553 - Task 1 + Task 2-2
…s param

When the name used in the child method was significantly more descriptive, the parameter is renamed, but the old name is still used within the function itself.

Related to 51553 - Task 1 + Task 2-2
…ed keyword as param

When the name used in the child method was significantly more descriptive, the parameter is renamed, but the old name is still used within the function itself.

Related to 51553 - Task 1 + Task 2-2
@jrfnl jrfnl force-pushed the trac-51553/mismatched-parameter-names branch from e0ffece to c829005 Compare July 9, 2021 17:41
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

I did a thorough deep code review multiple times during pair programming sessions with @jrfnl. The PR is ready for commit 👍

@hellofromtonya
Copy link
Contributor

Each atomic commit as been committed with the following changesets https://core.trac.wordpress.org/log/?revs=51728,51734-51735,51737,51739,51779-51790

@jrfnl jrfnl deleted the trac-51553/mismatched-parameter-names branch September 9, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants