This is a follow up to #1740000: Provide a generic entity row plugin. Currently comment, node row plugins that extend the entity base plugin are called 'View'. We need to improve this naming.

CommentFileSizeAuthor
#2 1817554.patch5.02 KBdamiankloip

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Issue tags: +Novice

How about NodeView, etc.? That would match the names of the API functions.

damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 1817554.patch5.02 KB

Hmm, I'm not sure. Me and @dawehener talked about this briefly yesterday and thought maybe NodeRow etc..

I personally think we should avoid using the word 'View' in these names. If we follow other core stuff, like field formatters, we should go with NodeRow, CommentRow...

Did a patch on the train. I also removed a random variable that somehow made it's way in the Entity class.

xjm’s picture

xjm
she/her
Primary language English
commented 
+++ b/core/modules/system/lib/Drupal/system/Plugin/views/row/EntityRow.phpundefined
@@ -13,8 +13,7 @@
-class Entity extends RowPluginBase {
-  protected $entityInfo1;
+class EntityRow extends RowPluginBase {

I confirmed that this is the only reference to this variable in core (don't you love saying that??).

That naming pattern seems reasonable to me. I haven't checked everything over yet; are there really no references to these classes outside the class declaration? Maybe that's the beauty of annotated plugins.

damiankloip’s picture

damiankloip commented

Yeah, It does keep things clean. It's all about that plugin ID! and of course it's the only reference to that variable :)

dawehner’s picture

dawehner
Primary language German
commented

I totally agree that these variable names help to understand the purpose of the file.

In general we should wait for a global renaming until short before code freeze.

xjm’s picture

xjm
she/her
Primary language English
commented

I'd actually disagree since it's just these few classes. The current names are really confusing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Oh i talked about renaming all the other classes... this should wait but for that patch i totally agree that everything else is damn confusing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This seems much clearer than before.

Committed and pushed to 8.x. Thanks!

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