Updated: Comment #6

Problem/Motivation

In ViewExecutable several methods dealing with Views handlers are using ...Item() instead. Especially as ViewExecutable::getItemsPerPage() and ViewExecutable::setItemsPerPage() are used in a total different context this may be very confusing.

Proposed resolution

Renaming the following methods to use "Handler" instead of "Item":

  • addItem() => addHandler()
  • generateItemId() => generateHandlerId()
  • getItems() => getHandlers()
  • getItem() => getHandler()
  • removeItem() => removeHandler()
  • setItemOption() => setHandlerOption()

Remaining tasks

-

User interface changes

-

API changes

Public methods of \Drupal\views\ViewExecutable are renamed:

  • addItem() => addHandler()
  • generateItemId() => generateHandlerId()
  • getItems() => getHandlers()
  • getItem() => getHandler()
  • removeItem() => removeHandler()
  • setItemOption() => setHandlerOption()

Developers dealing with handlers associated to a view need to rename their method calls.
The AddItem form is now called AddHandler.

CommentFileSizeAuthor
#24 interdiff.txt862 bytesdawehner
#24 vdc-2143495.patch68.11 KBdawehner
#20 vdc-2143495.patch67.73 KBdawehner
#18 interdiff.txt1.82 KBdawehner
#18 vdc-2143495.patch68.52 KBdawehner
#16 vdc-2143495.patch67.13 KBdawehner
#14 vdc-2143495.patch66.39 KBdawehner
#10 vdc-2143495.patch67.13 KBdawehner
#10 interdiff.txt30.92 KBdawehner
#6 viewexecutable-naming-item-to-handler-2143495-6.patch48.22 KBderhasi
#6 interdiff.txt22.03 KBderhasi
#2 viewexecutable-naming-item-to-handler-2143495-2.patch26.81 KBderhasi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

derhasi’s picture

derhasi CreditAttribution: derhasi commented
Issue summary: View changes
derhasi’s picture

derhasi CreditAttribution: derhasi commented
Issue summary: View changes
FileSize
viewexecutable-naming-item-to-handler-2143495-2.patch26.81 KB
derhasi’s picture

derhasi CreditAttribution: derhasi commented
Status: Active » Needs review

There's the patch ^^.

There are two things I very much would get feedback for:

  • I'm not sure if we should use "Handler" or "HandlerInstance" for the method names.
  • If \Drupal\views_ui\Form\Ajax\AddItem really only deals with handlers.
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I'm not sure if we should use "Handler" or "HandlerInstance" for the method names.

I do prefer using getHandler() over getHandlerInstance().

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

OH and yes, I am fine with renaming the AddItem form as well!

derhasi’s picture

derhasi CreditAttribution: derhasi commented
Issue summary: View changes
FileSize
interdiff.txt22.03 KB
viewexecutable-naming-item-to-handler-2143495-6.patch48.22 KB

There's the update with the Form changed and hopefully all references in description updated.

Status: Needs review » Needs work

The last submitted patch, 6: viewexecutable-naming-item-to-handler-2143495-6.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

6: viewexecutable-naming-item-to-handler-2143495-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: viewexecutable-naming-item-to-handler-2143495-6.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt30.92 KB
vdc-2143495.patch67.13 KB

Continued with doing the work on that and potentially fixed the test failure.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

+1 for the naming change.
I'll be back for a full review when it passes.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

I think this is pretty good already. Nice work.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
vdc-2143495.patch66.39 KB

Here is one.

Status: Needs review » Needs work

The last submitted patch, 14: vdc-2143495.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
vdc-2143495.patch67.13 KB

OH i missed the move in the last reroll.

Status: Needs review » Needs work

The last submitted patch, 16: vdc-2143495.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
vdc-2143495.patch68.52 KB
interdiff.txt1.82 KB

Let's fix the last one.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented 
+++ b/core/modules/views_ui/views_ui.routing.yml
@@ -120,14 +120,14 @@ views_ui.break_lock:
+  path: '/admin/structure/views/{js}/add-handler/{view}/{display_id}/{type}'

+++ b/core/modules/views_ui/views_ui.module
@@ -215,13 +215,13 @@ function views_ui_view_preview_section_handler_links(ViewExecutable $view, $type
+      'href' => "admin/structure/views/nojs/config-handler/{$view->storage->id()}/{$display['id']}/$type/$id",

+++ b/core/modules/views_ui/views_ui.routing.yml
@@ -204,38 +204,38 @@ views_ui.form_display:
+  path: '/admin/structure/views/{js}/config-handler-extra/{view}/{display_id}/{type}/{id}'

If we're changing these paths anyway, do you think it's worth us improving them slightly? Getting rid of the 'config-' part for starters maybe.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
vdc-2143495.patch67.73 KB

I would like to get the patch as straightforward as possible though I agree about the paths.
Damnit I hate that we don't use routes on there already. this would make things trivial.

Status: Needs review » Needs work

The last submitted patch, 20: vdc-2143495.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

20: vdc-2143495.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: vdc-2143495.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
vdc-2143495.patch68.11 KB
interdiff.txt862 bytes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: -DX +DX (Developer Experience)
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

24: vdc-2143495.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Priority: Normal » Major

Given how much this really make it easier to understand what getItem() is for.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

24: vdc-2143495.patch queued for re-testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Reviewed & tested by the community

Agreed, it really does. It's a big benefit for such a simple change.

derhasi’s picture

derhasi CreditAttribution: derhasi commented

Thanks for re-re-re-re-re-rolling. Great work!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Improve naming of ViewExecutable::addItem(), ::getItem() & Co. » Change notice: Improve naming of ViewExecutable::addItem(), ::getItem() & Co.
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

"Item" is a generic-enough word that agreed it doesn't make much sense to use it when there's another, more descriptive noun available. Especially if we're using "Item" elsewhere to mean something totally different.

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

Les Lim’s picture

Les Lim
he/they
Primary language English
CreditAttribution: Les Lim commented
Assigned: derhasi » Unassigned

Unassigning for the change notice writeup.

lexfunk’s picture

lexfunk CreditAttribution: lexfunk commented
Assigned: Unassigned » lexfunk

Assigning to myself for change notice writeup.

lexfunk’s picture

lexfunk CreditAttribution: lexfunk commented
Title: Change notice: Improve naming of ViewExecutable::addItem(), ::getItem() & Co. » Improve naming of ViewExecutable::addItem(), ::getItem() & Co.
Assigned: lexfunk » Unassigned
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

I created the change record: https://drupal.org/node/2181701.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Nice!

Status: Fixed » Closed (fixed)

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