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

Comments

derhasi’s picture

derhasi commented
Issue summary: View changes
derhasi’s picture

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

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
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
commented

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

derhasi’s picture

derhasi commented
Issue summary: View changes
StatusFileSize
new interdiff.txt22.03 KB
new 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
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
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt30.92 KB
new 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
commented

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

damiankloip’s picture

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
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 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
commented
Status: Needs work » Needs review
StatusFileSize
new 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
commented
Status: Needs work » Needs review
StatusFileSize
new vdc-2143495.patch68.52 KB
new interdiff.txt1.82 KB

Let's fix the last one.

damiankloip’s picture

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
commented
StatusFileSize
new 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
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
commented
Status: Needs work » Needs review
StatusFileSize
new vdc-2143495.patch68.11 KB
new interdiff.txt862 bytes
tim.plunkett’s picture

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

dawehner
Primary language German
commented

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

dawehner’s picture

dawehner
Primary language German
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
commented

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

damiankloip’s picture

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 commented

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
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
commented
Assigned: derhasi » Unassigned

Unassigning for the change notice writeup.

lexfunk’s picture

lexfunk commented
Assigned: Unassigned » lexfunk

Assigning to myself for change notice writeup.

lexfunk’s picture

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
commented

Nice!

Status: Fixed » Closed (fixed)

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