Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #2052421: [META] Rename Views properties to core standards
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Yes | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#64 | interdiff_62-64.txt | 1.12 KB | vsujeetkumar |
#64 | 2052591_64.patch | 108.34 KB | vsujeetkumar |
#62 | 2052591_62.patch | 107.41 KB | vsujeetkumar |
#55 | views-plugin-properites-2052591-55.patch | 109.67 KB | l0ke |
#40 | drupal8.views-module.FieldPluginBase.2052591-40.patch | 36.68 KB | l0ke |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedComment #2
YesCT CreditAttribution: YesCT commentedShould 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.
Comment #3
YesCT CreditAttribution: YesCT commentedAh, #1856630-17: [Change notice] [META] Rename Views methods to core standards mentions vars
Comment #5
dawehnerAll 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
This is the php syntax problem. You either need protected or public.
Comment #6
YesCT CreditAttribution: YesCT commentedcool.
this is "everything" including comments.
need to check things like the renders.
posting this first, then we will see what it changed too much.
Comment #7
YesCT CreditAttribution: YesCT commentedthese I thought could use a closer look.
Comment #9
YesCT CreditAttribution: YesCT commentedI'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.
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?
Comment #11
martin107 CreditAttribution: martin107 commentedSimple Reroll
Comment #13
martin107 CreditAttribution: martin107 commentedOne bug of many cleared.
in DataFieldRow.php there was an outstanding conversion of field_alias to fieldAlias.
Comment #17
martin107 CreditAttribution: martin107 commentedLast patch removed 1 error and 150 exceptions from test.
This patch is expected to remove 1 error!
Hoping someone else might have better luck.
Comment #19
headli CreditAttribution: headli commentedI've changed the status of the ticket back to "needs work".
Comment #20
martin107 CreditAttribution: martin107 commentedComment #21
JacobSanfordRemoving 'Needs reroll' as patch applies cleanly for me.
Comment #22
martin107 CreditAttribution: martin107 commentedHmm 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
Comment #23
JacobSanfordCompletely my error, I was squelching file-not-found errors. Resetting tag.
Comment #24
dcam CreditAttribution: dcam commentedUpdating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....
Comment #25
dbcollies CreditAttribution: dbcollies commentedI'm going to start working on this
Comment #26
dbcollies CreditAttribution: dbcollies commentedI'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.
Comment #27
dbcollies CreditAttribution: dbcollies commentedI misunderstood the standards. This issue still needs to be worked.
Comment #28
Sumeet.Pareek CreditAttribution: Sumeet.Pareek commentedRe-rolled the patch so that it cleanly applies now after the PSR4 changes. This helped - https://groups.drupal.org/node/424758
Comment #29
martin107 CreditAttribution: martin107 commentedtriggering testbot
Comment #31
aczietlow CreditAttribution: aczietlow commentedI'm going to start working on this.
Comment #32
aczietlow CreditAttribution: aczietlow commentedComment #33
martin107 CreditAttribution: martin107 commentedStraight reroll before I can work on any errors.
no conflicts just merging and auto-merging.
PS oh and cleared white space issue.
Comment #35
l0keComment #36
l0keI'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.
Comment #37
l0keComment #40
l0keSimple re-roll.
Comment #41
YesCT CreditAttribution: YesCT commentedI 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.
Comment #42
alexpottHow come this ->field_alias was not converted? It is
\Drupal\entity_reference\Plugin\views\display\EntityReference
. I just this is because we are not convertingRowPluginBase::$field_alias
- but this seems super fragile considering the patch does things like to row plugins.Comment #43
YesCT CreditAttribution: YesCT commentedthinking 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?
Comment #44
YesCT CreditAttribution: YesCT commentedI'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.
Comment #45
YesCT CreditAttribution: YesCT commentedNo 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
Comment #46
alexpottPostponing as per #43
Comment #47
alexpottComment #53
dawehnerThe parent is not longer postponed
Comment #54
l0keI 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 linesAs any other property converting to lowerCamelCase?
Comment #55
l0keI'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.
Comment #61
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedComment #62
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch created for 9.1.x, Please review.
Comment #64
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed more tests, Please review.
Comment #65
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedComment #72
quietone CreditAttribution: quietone at PreviousNext commentedThis is a coding standard fix and they are now done by sniff, not file. See #3346468: [meta] Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName