Similar to #1965164: Add a @PluginID annotation for plugins that only need an ID, like Views handlers let's also add additional annotations for all views plugins.
This belongs to the following meta issue: #1966246: [meta] Introduce specific annotations for each plugin type
The mapping for the different kind of plugin types is the following:
Plugin type | Annotation class |
---|---|
Display plugin | \Drupal\views\Plugin\Annotation\ViewsDisplay |
Cache plugin | \Drupal\views\Plugin\Annotation\ViewsCache |
Display Extender plugin | \Drupal\views\Plugin\Annotation\ViewsDisplayExtender |
Exposed Form plugin | \Drupal\views\Plugin\Annotation\ViewsExposedForm |
Query plugin | \Drupal\views\Plugin\Annotation\ViewsQuery |
Argument default plugin | \Drupal\views\Plugin\Annotation\ViewsArgumentDefault |
Argument validator plugin | \Drupal\views\Plugin\Annotation\ViewsArgumentValidator |
Access | \Drupal\views\Plugin\Annotation\ViewsAccess |
Style | \Drupal\views\Plugin\Annotation\ViewsStyle |
Row | \Drupal\views\Plugin\Annotation\ViewsRow |
Comment | File | Size | Author |
---|---|---|---|
#63 | vdc-1969388-63.patch | 90.7 KB | dawehner |
#63 | interdiff.txt | 993 bytes | dawehner |
#61 | vdc-1969388-61.patch | 89.73 KB | dawehner |
#54 | vdc-1969388-47.patch | 89.91 KB | dawehner |
#50 | vdc-1969388-50.patch | 89.86 KB | dawehner |
Comments
Comment #1
dawehnerAssign
Comment #1.0
dawehnerblub
Comment #1.1
dawehneradded some annotations
Comment #1.2
dawehnerremoved
usage
Comment #2
dawehnerThat's currently blocked on #1881606: Use a derivative to integrate all entities as row plugins as it puts in annotations, which we should not support for all row plugins.
Comment #2.0
dawehnera
Comment #3
dawehnerJust a rerole, though this is still blocked on the other issue.
This moves the annotation classes into Drupal\views\Annotation
It would be cool if someone could have a look at the dependencies of this patch.
Comment #4
dawehnerHere is a rerole now that the other patch landed.
Comment #6
dawehnerThere we go.
Comment #8
dawehnerJust a rerole.
Comment #10
olli CreditAttribution: olli commentedIsn't this still needed?
missing 'uses_route'?
missing 'short_title'?
Extra '\Plugin' and views dir is in $namespaces['Drupal\views'] i think.
Extra '\Plugin' and '\\Views' -> '\Views'
"views\$type" -> "views/$type"
Comment #11
dawehnerThis was a really really good review, thank you very much.
The key "no_uid" is not used at all anywhere, so let's skip it.
This now at least installs drupal, let's see how much else is left.
Comment #13
dawehnerFixed a couple of missing annotations.
Comment #15
olli CreditAttribution: olli commentedRight, that installs! One problem.., we need to pass derivative and module in get().
Also, looks like that short_title didn't get into viewspager::get() and the generic entity row plugin is missing too?
Comment #16
dawehnerGood catches.
Comment #18
olli CreditAttribution: olli commentedThis is now missing title and help. In general, do you want to make these translatables admin, title, short_title, help, ... required?
Why not parent::get() here?
Comment #19
dawehnerLet's allow all plugins to be extended with other annotations.
Comment #21
dawehnerThere we go.
Comment #23
olli CreditAttribution: olli commentedMissed one.
Comment #25
olli CreditAttribution: olli commented#23: vdc-1969388-23.patch queued for re-testing.
Comment #26
dawehnerNice!
Comment #27
tim.plunkettWe shouldn't override get(), we can just provide the defaults in line.
Also switching to DefaultPluginManager as it lets us kill ProcessDecorator.
Comment #29
tim.plunkett#27: vdc-1969388-27.patch queued for re-testing.
Comment #31
tim.plunkettBorrowing a hunk from #1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route.
Comment #32
dawehnerI know that this was done due to derivatives, but can't we skip the "(optional)" in the documentation so people don't get confused?
Comment #33
dawehnerJust rerolled the patch and removed optional where I think it confuses people. We are not doing that anywhere else in core as far as I know.
Comment #35
dawehnerComment #37
dawehnerdoh!.
Comment #38
tim.plunkettLet's get this in! Awesome work @dawehner.
Comment #39
alexpottI don't what "full" means in this comment. In discussion with @dawehner on IRC we realised that all views plugin annotations should have short_title key since views/PluginBase::pluginTitle() supports this. I would go with
The plugin title used in the views UI
Unnecessary comma and use of the word "Defines".
The standard here seems to be
Whether or not the plugin is selectable in the ui.
. I can not find any instances of Shouldunnecessary "it's"
Whether or not to use hook_menu() to register a route.
Comma here should be a semi-colon to be consistent.
A class to make the plugin derivative aware.
Whether or not to register a theme function automatically.
Comment #40
jibran#37: vdc-1969388-37.patch queued for re-testing.
Comment #42
dawehnerThanks for the good review. Here is a fix for it.
Comment #44
damiankloip CreditAttribution: damiankloip commentedWe should update this docblock to reflect multiple types.
Maybe 'whether the plugin should not be selectable in the UI' or something?
Do you think if these are not optional, we should not set an empty string here?
Seems like we could move more properties into here, like id, title, short_title (, help)?
Whether ...
Comment #45
dawehnerWell, it fails otherwise with derivatives, which don't really define a title in the base implementation.
We could be the full point of the special annotations is to have proper documentation per class. Otherwise we could really just use @Plugin and be happy with it.
Comment #47
dawehnerGood to know that we have test coverage for the title of styles/other plugins.
Comment #48
olli CreditAttribution: olli commentedNot sure if this is true anymore after #27.
Should we move from 'module' to 'provider'?
Comment #49
olli CreditAttribution: olli commentedAre these defaults still needed?
Comment #50
dawehnerLet's do that in a follow up, as this patch is already big enough, to be honest.
Good point!
Comment #52
tim.plunkett#50: vdc-1969388-50.patch queued for re-testing.
Comment #54
dawehnerLet's just readd the old patch.
Comment #55
olli CreditAttribution: olli commented#50: vdc-1969388-50.patch queued for re-testing.
Sorry for the noise, I dont see how those could be related to changes in the interdiff.
Comment #56
damiankloip CreditAttribution: damiankloip commentedI spoke to dawehner about the docs for properties on each annotation class etc.. Let's do this.
Comment #57
BerdirNot completely sure, is this an API change or not? Meaning, will the old annotation classes stop working when this gets in or not?
Comment #58
dawehnerIf you are strict this is an api change, that is right. Old views plugin will not work anymore.
Comment #59
dawehnerEclipseGC pointed out that because each annotation extends @Plugin, it will still work, if someone uses just @plugin. @ViewsPluginType is so just a nice way to document the available properties.
Comment #60
jibranAfter #1911492: Views try to find Custom StylePlugin template in core/modules/views/templates went in it needs reroll. Sorry @dawehner you made that when critical and RTBC. ;)
Comment #61
dawehnerRerolled.
Comment #63
dawehnerHa!
Comment #64
jibranBack to RTBC
Comment #65
damiankloip CreditAttribution: damiankloip commented+1 again, looks good
Comment #66
tstoeckler$id, $title and $short_title are used by all plugin types. Should they be on the base annotation class?
Also $no_ui is used by every plugin type but ViewsWizard so that might be a candidate as well, but probably a follow-up in that case.
Comment #67
dawehnerIt is okay to move no_ui to the base class, but I reject to move title and short_title on there as well. The point of the issue is to be able
to have a place for a proper documentation of this keys.
Moving this to a baseclass will reduce the possibility to find it. Once we are at this point, we can drop it entirely because it is not needed for the actual functionality.
Comment #68
tstoecklerAlso @damiankloip just opened the follow-up we need here :-): #2070609: Remove obsolete "module" keys from Views plugin annotations
Comment #69
damiankloip CreditAttribution: damiankloip commentedYep, done :-). See #44/#45 for previous discussion about moving stuff to the based class.
Comment #70
tstoecklerHmm... I don't actually think that this makes it more findable than in a base class, but I don't really care. Sorry, I had missed #44/#45 before.
Back to RTBC then.
Comment #71
damiankloip CreditAttribution: damiankloip commentedI'm in the same boat as you on that, but yeah, I don't mind either.
Comment #72
dawehnerTF.
Comment #73
alexpottCommitted 55bd232 and pushed to 8.x. Thanks!
I don't think this needs a core change notice because views is new. But it falls into the views change process.
Comment #74
dawehnerOh nice, thank you!!
Comment #75
tim.plunkettRemoving tags
Comment #75.0
tim.plunkettasdf
Comment #76
Chris Matthews CreditAttribution: Chris Matthews as a volunteer and at City of Oaks Design commentedFor more information as to why this issue was moved to the Drupal core project, please see issue #3030347: Plan to clean process issue queue
Comment #77
Chris Matthews CreditAttribution: Chris Matthews as a volunteer and at City of Oaks Design commentedMoving back to the contributed Views issue queue and closing as outdated per https://www.drupal.org/project/views/issues/3030347#comment-13023447