Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Psikik’s picture

Assigned: Unassigned » Psikik
Psikik’s picture

Converts the get_current_page() function to getCurrentPage and adds the public decorator.

Psikik’s picture

Assigned: Psikik » Unassigned
Status: Active » Needs review
kevin.reiss’s picture

Adding public keyword to method.

shoptalk’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice, -VDC

The last submitted patch, 2002898-get_current_page-conversion_4.patch, failed testing.

rootwork’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2002898-get_current_page-conversion_4.patch, failed testing.

rootwork’s picture

Status: Needs work » Needs review

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

The last submitted patch, 2002898-get_current_page-conversion_4.patch, failed testing.

rootwork’s picture

Assigned: Unassigned » rootwork

working on this

rootwork’s picture

Assigned: rootwork » Unassigned
Status: Needs work » Needs review
FileSize
6.56 KB
6.41 KB

OK, so I'm pretty sure the patch in #4 is failing because while getCurrentPage was set correctly, the relevant test is also relying on setCurrentPage, which was only partially renamed. The related issue is #2003292: Rename Views method set_current_page() to setCurrentPage().

So just for the purposes of seeing if this will pass the test, I've changed all the references to setCurrentPage as well. I'm attaching an interdiff. If this passes, then #4 will likely pass once #2003292: Rename Views method set_current_page() to setCurrentPage() lands (although conversely that might fail until this one lands).

heddn’s picture

Needs a re-roll. Instructions for "re-rolling" a new patch that applies cleanly.

$ git apply --index 2002898-get_current_page-conversion_12.patch
error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/field/Counter.php:50
error: core/modules/views/lib/Drupal/views/Plugin/views/field/Counter.php: patch does not apply
error: patch failed: core/modules/views/lib/Drupal/views/ViewExecutable.php:753
error: core/modules/views/lib/Drupal/views/ViewExecutable.php: patch does not apply

aspilicious’s picture

Status: Needs review » Needs work
somepal’s picture

Assigned: Unassigned » somepal

I am taking this.

somepal’s picture

Status: Needs work » Needs review
FileSize
4.15 KB

I might need to change the setter as well, but want to try this.

dawehner’s picture

Status: Needs review » Needs work

Thank you. Yeah please not do both at the same time.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined
@@ -103,7 +103,7 @@ function set_offset($offset) {
+  function getCurrentPage() {

Should be a public method.

somepal’s picture

Status: Needs work » Needs review
FileSize
4.16 KB

yeah, sorry I missed that. here's the re-roll, made it public.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much!

webchick’s picture

Title: Rename Views method get_current_page() to getCurrentPage() » Change notice: Rename Views method get_current_page() to getCurrentPage()
Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 8.x-3.x-dev
Component: views.module » Code
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks!

Moving to the Views queue for the change notice.

xjm’s picture

Title: Change notice: Rename Views method get_current_page() to getCurrentPage() » Rename Views method get_current_page() to getCurrentPage()
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Assigned: somepal » Unassigned
Status: Active » Fixed
Issue tags: -Needs change record

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