Part of meta-issue #2052421: [META] Rename Views properties to core standards

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Yes Instructions
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Assigned: Unassigned » YesCT
YesCT’s picture

Title: Copy of [META] Rename Views properties to core standards » In FieldPluginBase Rename Views properties to core standards
Status: Active » Needs review
FileSize
5.23 KB

Should check other classes that extend FieldPluginBase to see if they were using any of its properties?
If they are, we should rename them in those classes too?

There are a bunch of them
https://api.drupal.org/api/drupal/core%21modules%21views%21lib%21Drupal%...

Some of the name replacements were for "vars"
do they need any different treatment?

I gave the properties doc blocks too. Should we do that in general?
It was difficult to figure out what exactly the descriptions should be.

YesCT’s picture

Ah, #1856630-17: [Change notice] [META] Rename Views methods to core standards mentions vars

Note: The PHP 4 method of declaring a variable with the var keyword is still supported for compatibility reasons (as a synonym for the public keyword).

Status: Needs review » Needs work

The last submitted patch, drupal8.views-module.FieldPluginBase.2052591-2.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.phpundefined
@@ -51,22 +51,35 @@
+  protected $fieldAlias = 'unknown';
...
+  protected $aliases = array();

All this variables are used in potentially every subclass. In phpstorm you can rename variables with shift+f6 and rename all usages at the same time. MAGIC

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.phpundefined
@@ -51,22 +51,35 @@
+   $additionalFields = array();

This is the php syntax problem. You either need protected or public.

YesCT’s picture

Status: Needs work » Needs review
FileSize
29.84 KB
34.23 KB

cool.

this is "everything" including comments.
need to check things like the renders.

posting this first, then we will see what it changed too much.

YesCT’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/RowPluginBase.phpundefined
@@ -153,7 +153,7 @@ public function render($row) {
-      '#field_alias' => isset($this->field_alias) ? $this->field_alias : '',
+      '#fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/row/RssFields.phpundefined
@@ -179,7 +179,7 @@ public function render($row) {
-        'field_alias' => isset($this->field_alias) ? $this->field_alias : '',
+        'fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',

+++ b/core/modules/views/views.moduleundefined
@@ -100,7 +100,7 @@ function views_theme($existing, $type, $theme, $path) {
-    'row' => array('view' => NULL, 'options' => NULL, 'row' => NULL, 'field_alias' => NULL),
+    'row' => array('view' => NULL, 'options' => NULL, 'row' => NULL, 'fieldAlias' => NULL),

+++ b/core/modules/views/views.theme.incundefined
@@ -343,8 +343,8 @@ function template_preprocess_views_view_grouping(&$variables) {
+ * $field->fieldAlias says what the raw value in $row will be. Reach it like
+ * this: @code { $row->{$field->fieldAlias} @endcode

these I thought could use a closer look.

Status: Needs review » Needs work

The last submitted patch, drupal8.views-module.FieldPluginBase.2052591-6.patch, failed testing.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
2.74 KB
32.75 KB
537.36 KB

I'll be away from the computer for a while, and discussion is going on regarding some of this.
I'll take some notes and unassign in case someone wants to try something.

find-usages-twig.png

based on discussion between @dawehner @Cottser @berdir

"Implementation" http://twig.sensiolabs.org/doc/templates.html#variables
it just lowercases everything and compares, I don't see any conversion between under_score and camelCase https://github.com/fabpot/Twig/blob/master/lib/Twig/Template.php#L403

field_alias to fieldAlias would not match since it would convert fieldAlias to fieldalias

how can I know that the field_alias that's referenced in that twig template is referring to the one from FieldPluginBase?

{{ dump() }} in the template file might give you something useful but making that better is a work in progress over a couple of issues
#1783134: [META] Make it possible to inspect twig variables and get information about objects and render arrays and #1922304: Remove TwigReference objects in favor of a high speed implementation by using NodeVisitors more cleverly

In general though I would be looking at the hook_theme() definition, preprocess function, etc
This is a weird case though :)

views_theme:8?
views_theme: Implement hook_theme(). Register views theming functions. => views_theme($existing, $type, $theme, $path) => http://api.drupal.org/api/function/views_theme/8

the field_alias is part of a $variables array that is used in the foreach to register a theme hook for row style plugins
the @see there says to see template_preprocess_views_view_field()

for this we should leave #field_alias as underscores to be consistent with render API and just do:
isset($this->fieldAlias) ? $this->fieldAlias : '',
that way we would leave views_theme() as is...

If we had a row Twig template then it would get the 'field_alias' variable defined in views_theme()
we still need to update views-view-field though because that is accessing an object property

views_view_field() is just getting the row object, not going through row theming... FieldPluginBase::theme() which is called from StylePluginBase::renderFields()

row plugins do have a field alias
but it is not documented on the RowPluginBase class

it being set in the query method
in RowPluginBase
'#fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',

I think this should document (use?) FieldPluginBase

---
So...
leaving views-view-field.html.twig template with: * data = row[field.field_alias]
And changing to
'#field_alias' => isset($this->fieldAlias) ? $this->fieldAlias : '',
in RowPluginBase

--
is + * this: @code { $row->{$field->fieldAlias} @endcode
missing a bracket?

The last submitted patch, drupal8.views-module.FieldPluginBase.2052591-9.patch, failed testing.

martin107’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
29.96 KB
14.61 KB

Simple Reroll

The last submitted patch, 11: drupal8.views-module.FieldPluginBase.2052591-10.patch, failed testing.

martin107’s picture

One bug of many cleared.

in DataFieldRow.php there was an outstanding conversion of field_alias to fieldAlias.

Status: Needs review » Needs work

The last submitted patch, 13: interdiff-10-11.patch, failed testing.

The last submitted patch, 13: drupal8.views-module.FieldPluginBase.2052591-11.patch, failed testing.

The last submitted patch, 13: drupal8.views-module.FieldPluginBase.2052591-11.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
31.71 KB
970 bytes

Last patch removed 1 error and 150 exceptions from test.
This patch is expected to remove 1 error!

Hoping someone else might have better luck.

The last submitted patch, 17: drupal8.views-module.FieldPluginBase.2052591-12.patch, failed testing.

headli’s picture

Status: Needs review » Needs work

I've changed the status of the ticket back to "needs work".

martin107’s picture

Issue tags: +Needs reroll
JacobSanford’s picture

Issue tags: -Needs reroll

Removing 'Needs reroll' as patch applies cleanly for me.

martin107’s picture

Hmm this is what I get when I apply to the 8.x branch

It looks to me like the patch no longer applies

Jacob could you post what commit you tested against ?

mine was

commit 1167235b96d773457f26f952368f4fa643d36e00
Author: Jennifer Hodgdon
Date: Mon Jun 2 10:24:20 2014 -0700

and the result of

git apply drupal8.views-module.FieldPluginBase.2052591-12.patch

was

drupal8.views-module.FieldPluginBase.2052591-12.patch:444: trailing whitespace.
 error: core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/field/TitleLink.php: No such file or directory
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/Comment.php: No such file or directory
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/LastTimestamp.php: No such file or directory
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.php: No such file or directory
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/Username.php: No such file or directory
error: core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php: No such file or directory
error: core/modules/file/lib/Drupal/file/Plugin/views/field/File.php: No such file or directory
error: core/modules/file/lib/Drupal/file/Plugin/views/field/FileMime.php: No such file or directory
error: core/modules/file/lib/Drupal/file/Plugin/views/field/Uri.php: No such file or directory
error: core/modules/history/lib/Drupal/history/Plugin/views/field/HistoryUserTimestamp.php: No such file or directory
error: core/modules/node/lib/Drupal/node/Plugin/views/field/Node.php: No such file or directory
error: core/modules/node/lib/Drupal/node/Plugin/views/field/Path.php: No such file or directory
error: core/modules/node/lib/Drupal/node/Plugin/views/field/Revision.php: No such file or directory
error: core/modules/node/lib/Drupal/node/Plugin/views/field/RevisionLink.php: No such file or directory
error: core/modules/rest/lib/Drupal/rest/Plugin/views/row/DataFieldRow.php: No such file or directory
error: core/modules/search/lib/Drupal/search/Plugin/views/field/Score.php: No such file or directory
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/field/LinkEdit.php: No such file or directory
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/field/Taxonomy.php: No such file or directory
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/field/TaxonomyIndexTid.php: No such file or directory
error: core/modules/user/lib/Drupal/user/Plugin/views/field/Link.php: No such file or directory
error: core/modules/user/lib/Drupal/user/Plugin/views/field/Name.php: No such file or directory
error: core/modules/user/lib/Drupal/user/Plugin/views/field/Permissions.php: No such file or directory
error: core/modules/user/lib/Drupal/user/Plugin/views/field/Roles.php: No such file or directory
error: core/modules/user/lib/Drupal/user/Plugin/views/field/User.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/EntityLabel.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/MachineName.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/Markup.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/Serialized.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/field/TimeInterval.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/row/RowPluginBase.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Plugin/views/row/RssFields.php: No such file or directory
error: core/modules/views/lib/Drupal/views/Tests/Handler/FieldUnitTest.php: No such file or directory
JacobSanford’s picture

Issue tags: +Needs reroll

Completely my error, I was squelching file-not-found errors. Resetting tag.

dcam’s picture

Issue summary: View changes

Updating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

dbcollies’s picture

I'm going to start working on this

dbcollies’s picture

Status: Needs work » Closed (fixed)
Related issues: +#2052421: [META] Rename Views properties to core standards

I've reviewed this, and it appears the work has already been completed. The only issue I found was with the attribute tableAlias, which is inherited from Drupal\views\Plugin\views\HandlerBase.

dbcollies’s picture

Status: Closed (fixed) » Needs work

I misunderstood the standards. This issue still needs to be worked.

Sumeet.Pareek’s picture

Re-rolled the patch so that it cleanly applies now after the PSR4 changes. This helped - https://groups.drupal.org/node/424758

martin107’s picture

Status: Needs work » Needs review

triggering testbot

Status: Needs review » Needs work

The last submitted patch, 28: drupal8.views-module.FieldPluginBase.2052591-28.patch, failed testing.

aczietlow’s picture

Assigned: Unassigned » aczietlow

I'm going to start working on this.

aczietlow’s picture

Assigned: aczietlow » Unassigned
martin107’s picture

Status: Needs work » Needs review
FileSize
33.87 KB

Straight reroll before I can work on any errors.

no conflicts just merging and auto-merging.

PS oh and cleared white space issue.

Status: Needs review » Needs work

The last submitted patch, 33: drupal8.views-module.FieldPluginBase.2052591-33.patch, failed testing.

l0ke’s picture

Assigned: Unassigned » l0ke
l0ke’s picture

Status: Needs work » Needs review
FileSize
36.83 KB
14.98 KB

I've made the patch from scratch, a lot of stuff were changed.
I think there is no reason to make fieldAlias and aliases protected, otherwise we should provide getters and setters that might should be a separate issue.
Also tests should now pass.

l0ke’s picture

Issue tags: +#ams2014contest

Status: Needs review » Needs work

The last submitted patch, 36: drupal8.views-module.FieldPluginBase.2052591-36.patch, failed testing.

l0ke’s picture

Status: Needs work » Needs review
Issue tags: - +Amsterdam2014
FileSize
36.68 KB

Simple re-roll.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I read through the patch, also applied it and looked at it in my IDE. looks good.

if things need to be protected, let's do that in a separate issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
      // Build the condition using the selected search fields.
      foreach ($style_options['options']['search_fields'] as $field_alias) {
        if (!empty($field_alias)) {
          // Get the table and field names for the checked field.
          $field = $this->view->query->fields[$this->view->field[$field_alias]->field_alias];
          // Add an OR condition for the field.
          $conditions->condition($field['table'] . '.' . $field['field'], $value, 'LIKE');
        }
      }

How come this ->field_alias was not converted? It is \Drupal\entity_reference\Plugin\views\display\EntityReference. I just this is because we are not converting RowPluginBase::$field_alias - but this seems super fragile considering the patch does things like to row plugins.

+++ b/core/modules/views/src/Plugin/views/row/OpmlFields.php
@@ -200,7 +200,7 @@ public function render($row) {
-      '#field_alias' => isset($this->field_alias) ? $this->field_alias : '',
+      '#fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',

+++ b/core/modules/views/src/Plugin/views/row/RowPluginBase.php
@@ -175,7 +175,7 @@ public function render($row) {
-      '#field_alias' => isset($this->field_alias) ? $this->field_alias : '',
+      '#fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -178,7 +178,7 @@ public function render($row) {
-      '#field_alias' => isset($this->field_alias) ? $this->field_alias : '',
+      '#fieldAlias' => isset($this->field_alias) ? $this->field_alias : '',
YesCT’s picture

thinking through this

alexpott points out that in OpmlFields there is a $this->field_alias and that it might should be converted in this issue also.
I looked in OpmlFields
there are some $this->words where words are not defined properties on the OpmlFields class, but are known...
like $this->options which comes from the PluginBase
class OpmlFields extends RowPluginBase
abstract class RowPluginBase extends PluginBase
but where is $this->field_alias coming from?
seems like it might be a magic property being set on OpmlFields

should it also be extending something else that has field_alias property? or should itself have a field_alias property?

YesCT’s picture

I'm not sure if the last bit of #43 is a bug that needs a separate issue or not.

I was going to mark this postponed according to https://www.drupal.org/contribute/core/beta-changes
but it might get in under the amsterdam exception.. except it has been more than a month now.

YesCT’s picture

Issue tags: -Novice

No novice task on this anymore, and we dont even know if it should be worked on right now, so removing the novice tag per https://www.drupal.org/core-mentoring/novice-tasks

alexpott’s picture

Status: Needs work » Postponed

Postponing as per #43

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Postponed » Needs work

The parent is not longer postponed

l0ke’s picture

I have a question regarding the properties declared dynamically. How do we properly handle them?

For example core/modules/comment/src/Plugin/views/field/StatisticsLastCommentName.php has the next lines

    // nes_user alias so this can work with the sort handler, below.
    $this->user_table = $this->query->ensureTable('ces_users', $this->relationship, $join);

    $this->fieldAlias = $this->query->addField(NULL, "COALESCE($this->user_table.name, $this->tableAlias.$this->field)", $this->tableAlias . '_' . $this->field);

    $this->user_field = $this->query->addField($this->user_table, 'name');
    $this->uid = $this->query->addField($this->tableAlias, 'last_comment_uid');

As any other property converting to lowerCamelCase?

l0ke’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysLisbon
FileSize
109.67 KB

I'm not sure I have chosen a right way but I've tried to replace all dynamic declarations as well as some private methods.
Want to give it a try.

Status: Needs review » Needs work

The last submitted patch, 55: views-plugin-properites-2052591-55.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

vsujeetkumar’s picture

Assigned: l0ke » Unassigned
Issue tags: +Needs reroll
vsujeetkumar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
107.41 KB

Re-roll patch created for 9.1.x, Please review.

Status: Needs review » Needs work

The last submitted patch, 62: 2052591_62.patch, failed testing. View results

vsujeetkumar’s picture

Fixed more tests, Please review.

vsujeetkumar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: 2052591_64.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Postponed

This is a coding standard fix and they are now done by sniff, not file. See #3346468: [meta] Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.