Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markie’s picture

Assigned: Unassigned » markie
Status: Active » Needs review
FileSize
1.11 KB

patched

Status: Needs review » Needs work
Issue tags: -Novice, -VDC

The last submitted patch, rename_getFieldAlias-2002912-1.patch, failed testing.

oenie’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +VDC
oenie’s picture

Patch queued for a retest, apart from problem below it should be in order.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1046,7 +1046,7 @@ function add_groupby($clause) {
+  function getFieldAlias($table_alias, $field) {

Add protected access modifier in front of the function to adher to the new OOP standards.

oenie’s picture

Status: Needs review » Needs work
markie’s picture

Status: Needs work » Needs review
FileSize
1.22 KB

protected access added, but can you tell me why that function should be protected?

oenie’s picture

Sure. I'm going to assume you want to know the difference between the two ?
(You're a longtime Drupaller, so i'm not sure this is the right assumption, but anyway :))

Protected is used when methods are for internal use of an object and its subclasses.
In this case, you can see that the only code that calls it is $this->getFieldAlias.
Which would mean protected is enough for access.

This in contrast to code where you would see things like:

$this->query->getFieldAlias(...)

Here another class is calling the method on a different class. If you would write protected there, it would field a problem because the method is not visible to it. Public would be needed there.

Although it's not sure this mehod will always be called internally, i would always choose for the less visible one.
Unless of course it would be something more obvious (which this one doesn't feel like to me)

markie’s picture

Hey thanks for the reply. I just wanted to hear your reasoning. Glad you went into detail.

Does everything look good?

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Applies cleanly and no mentions of get_field_alias any more. Passed testbot.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5c63406 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.