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

[4.0] Use null coalescing operator where applicable #30118

Closed
wants to merge 4 commits into from
Closed

[4.0] Use null coalescing operator where applicable #30118

wants to merge 4 commits into from

Conversation

SharkyKZ
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

Use null coalescing operator over ternary operator and instanceof checks in methods with nullable class typehints.

Testing Instructions

  1. Code review.
  2. Browse around in frontend and backend.

Expected result AFTER applying this Pull Request

  1. Joomla still works.

Documentation Changes Required

No.

@richard67
Copy link
Member

Fore some reason this PR doesn't appear in the issue tracker? @zero-24 Any idea? Or who could know?

@@ -69,7 +69,7 @@ public static function orderbyPrimary($orderby)
*/
public static function orderbySecondary($orderby, $orderDate = 'created', DatabaseInterface $db = null)
{
$db = $db ?: Factory::getDbo();
$db = $db ?? Factory::getDbo();
Copy link
Contributor

Choose a reason for hiding this comment

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

?? is essentially a shorthand for isset($foo) && !empty($foo) ? $foo : $bar

The $db variable is always set, even if null. So the shorthand ?: is correct here.

Null coalescing really should only be used in cases where it is uncertain if a reference (array key or variable) will be defined.

All of that to say that what’s there pre-patch is correct for these types of uses and while the null coalescing operator might work to me it raises a flag in a code review because then I’m looking for a code path where something on the left side isn’t defined at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?? is shorthand for isset(). It allows anything but null. Both ?? are ?: fine in terms of functionality. The idea here was to use ?? sort of as a strict comparison against null. But maybe just use === null instead. Anyways I'm just nitpicking here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The $db = $db instanceof DatabaseDriver ? $db : Factory::getDbo(); stuff absolutely should be shorthanded to $db = $db ?: Factory::getDbo();.

But personally, I would favor ?: over ?? always unless you are dealing with potentially unassigned variables or array keys or things like that.

So in places where there are constructors with a signature of __construct(array $config), those are the perfect places to use ?? to handle things like $this->foo = $config['foo'] ?? $saneDefault;.

I get in all of the cases in this PR that ?? and ?: result in functionally equivalent code, but to people actually reading the code and understanding the operators, whenever I see ?? personally I'm next looking to see why it's shorthanding the left side of that operation needs some form of isset check (and no, I don't consider isset checks as necessary on $foo = null assignments, those variables are actually defined but they don't have values).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me ?? looks like a strict comparison against null. But I guess it's a matter of preference. Going to close this.

@Quy Quy added the PR-4.0-dev label Jul 17, 2020
@SharkyKZ SharkyKZ closed this Jul 19, 2020
@SharkyKZ SharkyKZ deleted the j4/cleanup/null branch July 19, 2020 09:53
@zero-24
Copy link
Contributor

zero-24 commented Jul 21, 2020

Fore some reason this PR doesn't appear in the issue tracker? @zero-24 Any idea? Or who could know?

Please open for such issues an issue here: joomla/jissues. I have no access to the issue tracker to check the reason behind it nor do i want to have any access there ;) The last time it was an issue with webhooks that run on error and it seemed to be an upstream issue from GitHub but best is to open an issue there so this can be checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants