Posted by dawehner on November 24, 2012 at 11:31am
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Performance, VDC |
Issue Summary
Problem/Motivation
While performance testing on #1811816: Benchmark node_page_default vs. node view one issue was that PluginBase::setOptionDefaults was rather slow.
Proposed resolution
- Remove the unneeded translation (discuss whether it makes sense to translate default values once we have metadata config translation in)
- Simplify the code by requiring a default option (which is actually the case since d6.3)
Comments
#1
#2
The last submitted patch, drupal-1849280-1.patch, failed testing.
#3
Yeah this shows some bugs in defineOptions implementations.
#4
The last submitted patch, drupal-1849280-3.patch, failed testing.
#5
This time with a proper default value for the boolean handler.
#6
#5: drupal-1849280-5.patch queued for re-testing.
#7
The last submitted patch, drupal-1849280-5.patch, failed testing.
#8
#5: drupal-1849280-5.patch queued for re-testing.
#9
The last submitted patch, drupal-1849280-5.patch, failed testing.
#10
#5: drupal-1849280-5.patch queued for re-testing.
#11
The last submitted patch, drupal-1849280-5.patch, failed testing.
#12
#5: drupal-1849280-5.patch queued for re-testing.
#13
#5: drupal-1849280-5.patch queued for re-testing.
#14
The last submitted patch, drupal-1849280-5.patch, failed testing.
#15
This contained some other patch internally.
#16
The last submitted patch, drupal-1849280-15.patch, failed testing.
#17
#15: drupal-1849280-15.patch queued for re-testing.
#18
#15: drupal-1849280-15.patch queued for re-testing.
#19
The last submitted patch, drupal-1849280-15.patch, failed testing.
#20
Let's fix it.
#21
#20: drupal-1849280-20.patch queued for re-testing.
#22
The last submitted patch, drupal-1849280-20.patch, failed testing.
#23
Just another rerole
#24
The last submitted patch, drupal-1849280-23.patch, failed testing.
#25
#23: drupal-1849280-23.patch queued for re-testing.
#26
The changes look great! Two small nitpicks
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+ * Fill up the options of the plugin with defaults.
Fills
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+ * An array which stores the actual option values of the plugin.
+ *
+ * @param array $options
No blank line needed
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined@@ -99,17 +99,27 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+ * - default: The default value of one option
+ * - contains: (optional) An array which describes the available options
+ * under the key.
These are indented 2 spaces too much, and the (optional) goes before the key name I think
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.phpundefined@@ -38,7 +38,7 @@ protected function defineOptions() {
- $options['not'] = array('definition bool' => 'reverse');
+ $options['not'] = array('default' => FALSE, 'bool' => TRUE);
Hahaha, what was that? :)
#27
#23: drupal-1849280-23.patch queued for re-testing.
#28
Yeah indeed this patch showed some odd bugs in the code :)
Thanks for the review!
#29
The removed $level param hasn't been used since Views 6.x-2.x, so in theory this clean-up could get backported.
Nice work @dawehner!
#30
We probably could only fix the $level parameter, but not the rest, as people might have broken option_defintions(),
so doesn't seems worth to do.
#31
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined@@ -99,17 +99,26 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
if (isset($definition['contains']) && is_array($definition['contains'])) {
If we are trimming the fat out of this method, should/could we also assume that if 'contains' is set, it will be an array.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.phpundefined@@ -99,17 +99,26 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+ * - default: The default value of one option
+ * - (optional) contains: An array which describes the available options
+ * under the key.
Small thing: Should we add the translatable key to the docs too?
I have also just opened #1922966: remove 'bool' key from option definitions as a related follow up.
#32
Oops, must have x-posted
#33
Good points.
#34
Yay performance!
Committed and pushed to 8.x. Thanks!
#35
Automatically closed -- issue fixed for 2 weeks with no activity.