Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
phenaproxima’s picture

Status: Active » Needs review
FileSize
16.79 KB
phenaproxima’s picture

Assigned: phenaproxima » Unassigned
dawehner’s picture

Thanks for this big conversion.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1044,7 +1044,7 @@ function add_groupby($clause) {
+   * @see views_plugin_query_default::addField()

This should be \Drupal\views\Plugin\views\query\Sql::addField

rdrh555’s picture

Assigned: Unassigned » rdrh555
sassafrass’s picture

Assigned: rdrh555 » Unassigned
Status: Needs review » Reviewed & tested by the community

Applied patch successfully and did some cursory testing of addField function.

oenie’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -743,7 +743,7 @@ function get_table_info($table) {
+  function addField($table, $field, $alias = '', $params = array()) {

Add public access modifier in front of the method to adher to the new OOP standards.

connorwk’s picture

Assigned: Unassigned » connorwk
connorwk’s picture

Status: Needs work » Needs review
FileSize
16.81 KB

Fixed what dawehner and oenie pointed out.

rdrh555’s picture

After a search, all remaining 'add_fields' are subsets of other function names. Made @dawehner's change.

rdrh555’s picture

Assigned: rdrh555 » Unassigned

Disregard #10 patch; this was unassigned from me while I was working on it (#6), not quite sure why...
@connork didn't mean to change assigned, please assign back to yourself.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looks ok to me

oenie’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -743,7 +743,7 @@ function get_table_info($table) {
+  function addField($table, $field, $alias = '', $params = array()) {

Hate to kick this back again, but there's still a public access modifier missing.

aspilicious’s picture

whooops, thnx oenie

zschmid’s picture

Status: Needs work » Needs review
FileSize
18.1 KB

added public access modifier

oenie’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, apart from a minor comment problem, still referencing the old method:

core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.php, line 799:

* - add_field: add a 'num_nodes' field for the count. Usually it will

elvis2’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
17.21 KB

Re-roll. Fixed a few things.

oenie’s picture

Status: Needs review » Reviewed & tested by the community

Looks go to me now.
You just nicely pointed out another problem that i might not have spotted in other reviews though (references in the comments) :/
But this one looks fine.

catch’s picture

Issue tags: -Novice, -VDC

#17: 2002144-17-rename-add_field.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2002144-17-rename-add_field.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#17: 2002144-17-rename-add_field.patch queued for re-testing.

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

The last submitted patch, 2002144-17-rename-add_field.patch, failed testing.

elvis2’s picture

Looks like this needs a re-roll due to the updated code.

drupalway’s picture

Assigned: Unassigned » drupalway
Issue tags: +CodeSprintUA

We are working today with this issue during Code Sprint UA

drupalway’s picture

Status: Needs work » Needs review
FileSize
17.09 KB

Re-roll. Please review.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#25 looks good for me
if bot happy - rtbc!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-2002144-views-addField-25.patch, failed testing.

star-szr’s picture

Patch in #25 is p0 formatted, should be p1 formatted.

See https://drupal.org/node/707484 for more information.

We'll need to create a new patch file based on #25 by applying the patch locally with patch -p0 > patchname.patch and then generating a new patch file. @drupalway or someone else?

heddn’s picture

Status: Needs work » Needs review
FileSize
18.57 KB

Let's see if the testbot likes us.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

oenie’s picture

Looks good to me !

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ba4d4a8 and pushed to 8.x. Thanks!

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