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
Files: 
CommentFileSizeAuthor
#40 drupal8.views-module.FieldPluginBase.2052591-40.patch36.68 KBlokeoke
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,023 pass(es).
[ View ]
#36 interdiff-2052591-33-36.txt14.98 KBlokeoke
#36 drupal8.views-module.FieldPluginBase.2052591-36.patch36.83 KBlokeoke
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.views-module.FieldPluginBase.2052591-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 drupal8.views-module.FieldPluginBase.2052591-33.patch33.87 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,189 pass(es), 13 fail(s), and 3 exception(s).
[ View ]
#28 drupal8.views-module.FieldPluginBase.2052591-28.patch33.87 KBSumeet.Pareek
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,553 pass(es), 13 fail(s), and 3 exception(s).
[ View ]
#17 interdiff-11-12.txt970 bytesmartin107
#17 drupal8.views-module.FieldPluginBase.2052591-12.patch31.71 KBmartin107
FAILED: [[SimpleTest]]: [MySQL] 59,481 pass(es), 12 fail(s), and 3 exception(s).
[ View ]
#13 interdiff-10-11.patch908 bytesmartin107
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 drupal8.views-module.FieldPluginBase.2052591-11.patch30.81 KBmartin107
FAILED: [[SimpleTest]]: [MySQL] 59,477 pass(es), 12 fail(s), and 3 exception(s).
[ View ]
#11 interdiff.txt14.61 KBmartin107
#11 drupal8.views-module.FieldPluginBase.2052591-10.patch29.96 KBmartin107
FAILED: [[SimpleTest]]: [MySQL] 59,508 pass(es), 13 fail(s), and 153 exception(s).
[ View ]
#9 find-usages-twig.png537.36 KBYesCT
#9 drupal8.views-module.FieldPluginBase.2052591-9.patch32.75 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,177 pass(es), 6 fail(s), and 150 exception(s).
[ View ]
#9 interdiff-6-9.txt2.74 KBYesCT
#6 drupal8.views-module.FieldPluginBase.2052591-6.patch34.23 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,680 pass(es), 6 fail(s), and 150 exception(s).
[ View ]
#6 interdiff-2-6.txt29.84 KBYesCT
#2 drupal8.views-module.FieldPluginBase.2052591-2.patch5.23 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php.
[ View ]

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
StatusFileSize
new5.23 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php.
[ View ]

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
StatusFileSize
new29.84 KB
new34.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,680 pass(es), 6 fail(s), and 150 exception(s).
[ View ]

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
StatusFileSize
new2.74 KB
new32.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,177 pass(es), 6 fail(s), and 150 exception(s).
[ View ]
new537.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
StatusFileSize
new29.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,508 pass(es), 13 fail(s), and 153 exception(s).
[ View ]
new14.61 KB

Simple Reroll

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

martin107’s picture

StatusFileSize
new30.81 KB
FAILED: [[SimpleTest]]: [MySQL] 59,477 pass(es), 12 fail(s), and 3 exception(s).
[ View ]
new908 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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
StatusFileSize
new31.71 KB
FAILED: [[SimpleTest]]: [MySQL] 59,481 pass(es), 12 fail(s), and 3 exception(s).
[ View ]
new970 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.

headly’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

Issue tags:-Needs reroll
StatusFileSize
new33.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,553 pass(es), 13 fail(s), and 3 exception(s).
[ View ]

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
StatusFileSize
new33.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,189 pass(es), 13 fail(s), and 3 exception(s).
[ View ]

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.

lokeoke’s picture

Assigned:Unassigned» lokeoke
lokeoke’s picture

Status:Needs work» Needs review
StatusFileSize
new36.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.views-module.FieldPluginBase.2052591-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new14.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.

lokeoke’s picture

Issue tags:+#ams2014contest

Status:Needs review» Needs work

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

lokeoke’s picture

Status:Needs work» Needs review
Issue tags:-+Amsterdam2014
StatusFileSize
new36.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,023 pass(es).
[ View ]

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