Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
\Drupal\views\ViewExecutable::$style_plugin
and \Drupal\views\ViewExecutable::$rowPlugin
should have corresponding methods, one could call.
Once we do that, we can also mark the public properties as deprecated
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff_50-52.txt | 14.84 KB | rahulkhandelwal1990 |
#52 | 2719173-52.patch | 21.59 KB | rahulkhandelwal1990 |
#49 | interdiff_48-49.txt | 1.88 KB | varshith |
#49 | 2719173-49.patch | 2.19 KB | varshith |
#48 | interdiff_41-48.txt | 1.97 KB | rahulkhandelwal1990 |
Comments
Comment #2
zauravWorking on this at the DrupalConNOLA Sprint
Comment #3
zauravFirst try
Comment #4
rakesh.gectcr@zaurav
Next time Upload the patch , Please update the status to review 'Needs review'.
Comment #5
mikeker CreditAttribution: mikeker as a volunteer commented@zaurav, thank for working on this!
These should be protected so that they can be overridden by child classes. Other than that, it looks great.
Comment #7
mikeker CreditAttribution: mikeker as a volunteer commentedHeh, completely spaced on this... These need to remain public for now, but marked as @deprecated (as you have done). Since we shipped 8.0 with these as public methods, it's an API change to change that.
Comment #8
zaurav@mikeker, thank you for helping me.
2nd try.
Comment #9
mikeker CreditAttribution: mikeker as a volunteer commentedLooks like a trailing whitespace got in the last change...
Comment #10
dawehnerLet's not rename this function ...
Comment #11
mikeker CreditAttribution: mikeker as a volunteer commentedSorry, missed this the first time. Technically, we're deprecating this in 8.2.x.
Comment #13
zauravImplemented changes from #9 #10 #11.
Unsure what to return in the case of $rowPlugin empty.
Comment #14
mikeker CreditAttribution: mikeker as a volunteer commented@zaurav, Don't forget to set the status to Needs Review so the testbot tests the new patch.
Tagging for NOLA sprints. I mentored @zaurav on this issue.
Comment #15
Mile23I'm pretty sure we can't deprecate for 8.2.x, and need to target 9.0.0, particularly for a getter method.
Also I doubt we'd really be *removing* these properties. Maybe say we'll be removing access to them.
The accessor doc line should match the property doc line, with 'Gets...' at the beginning.
Since we can return NULL, then
@return
should be\Drupal\views\Plugin\views\row\RowPluginBase|null
and explain when the method will return NULL.Comment #16
Vinay15Comment #17
Vinay15Hi Paul, I have worked on your suggestions and updated the patch.
Comment #18
dawehnerThank you for picking up the issue!
Should be '8.2.x' I think :)
Let's
getStyle()
call togetStylePlugin()
This should initialize the row plugin otherwise, IMHO, so use
$display->getPlugin('row')
. Once this is done,\Drupal\views\Plugin\views\style\StylePluginBase::init
could call out to just$view->getRowPlugin()
Comment #19
Vinay15Hi Daniel, I have tried to update the patch with your suggestions. Please correct me if I am wrong.
Comment #20
mikeker CreditAttribution: mikeker as a volunteer commented@Vinay15, thank you for your work on this issue!
Went the wrong way on this one. :) When @dawehner said it should be 8.2.x, he meant it should read "@deprecated as of Drupal 8.2.x and access will be removed in Drupal 9.0.0."
We can't actually change the access during the 8.x cycle as that would break backwards compatibility. So we mark as deprecated (as of 8.2.x) and change access to protected in the 9.x branch.
Similar to the above, but should read "@deprecated as of Drupal 8.2.x and will be removed in Drupal 9.0.0."
In this case the old method will be removed -- in the previous cases the property will remain but will go from public to protected.
Sorry for the nitpicks, but getting the version numbers correct is important when deprecating things.
Comment #21
Vinay15Hey Mike, thanks a lot for correcting me.. :) I have updated the patch.
Comment #24
ZeiP CreditAttribution: ZeiP as a volunteer commentedRe-rolling against 8.4.x.
Comment #28
joachim CreditAttribution: joachim as a volunteer commentedThe version numbers need updating again, sadly.
Docs should say what to use instead. I know it's in the source 2 lines lower, but on api.d.org, you only see the source if you click the 'show source' button.
Comment #29
Eli-TComment #30
aburrows CreditAttribution: aburrows at DigiDrop commentedWorking on this at DistributedSprintUK18
Comment #31
aburrows CreditAttribution: aburrows at DigiDrop commentedLatest patch rerolled with 8.7x added
Comment #33
joachim CreditAttribution: joachim as a volunteer commented#28.2 still needs fixing.
Comment #34
hardikpandya CreditAttribution: hardikpandya at Trigyn Technologies Ltd commentedComment #36
shubhangi1995Comment #37
joachim CreditAttribution: joachim as a volunteer commentedLatest patch looks good, but I don't think it'll make it into 8.7.x, so the comments will need changing to say 8.8.x.
Comment #38
vacho CreditAttribution: vacho at Skilld commentedIf the patch deprecate the variable $rowPlugin why the new code use it?
Comment #39
joachim CreditAttribution: joachim as a volunteer commentedBecause it's going to remain as an internal property. The point of this issue is to add accessors for these properties so that in D9 they can be changed to protected. The class itself can and should still refer to it.
Comment #41
mrweiner CreditAttribution: mrweiner as a volunteer commentedUpdate per #37 to update references from 8.7.x to 8.8.x
Comment #47
surabhi-gokte CreditAttribution: surabhi-gokte commentedComment #48
rahulkhandelwal1990 CreditAttribution: rahulkhandelwal1990 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedCreated a new patch and resolved Custom Commands Failed for 9.3.x in #41
Comment #49
varshith CreditAttribution: varshith as a volunteer and commentedFormatting fixes and fix issue link in doc.
Comment #50
rahulkhandelwal1990 CreditAttribution: rahulkhandelwal1990 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAdded one missing file from last patch(#41)
Comment #51
rahulkhandelwal1990 CreditAttribution: rahulkhandelwal1990 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #52
rahulkhandelwal1990 CreditAttribution: rahulkhandelwal1990 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedGetting below phpunit issues after this patch
1x: The "PHPUnit\TextUI\ResultPrinter" class is considered internal This class is not covered by the backward compatibility promise for PHPUnit. It may change without further notice. You should not use it from "Drupal\Tests\Listeners\HtmlOutputPrinter".
1x in DrupalListener::endTest from Drupal\Tests\Listeners
1x: The "Drupal\Tests\Listeners\DrupalListener" class implements "PHPUnit\Framework\TestListener" that is deprecated Use the `TestHook` interfaces instead.
1x in DrupalListener::endTest from Drupal\Tests\Listeners
1x: The "Drupal\Tests\Listeners\DrupalListener" class uses "PHPUnit\Framework\TestListenerDefaultImplementation" that is deprecated The `TestListener` interface is deprecated.
1x in DrupalListener::endTest from Drupal\Tests\Listeners
Seems these are deprecation issues due to phpunit version. See https://www.drupal.org/project/drupal/issues/3125809
Comment #53
rahulkhandelwal1990 CreditAttribution: rahulkhandelwal1990 as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedInterdiff of #52