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)
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | interdiff.txt | 1.21 KB | dawehner |
| #33 | drupal-1849280-33.patch | 3.72 KB | dawehner |
| #28 | drupal-1849280-28.patch | 3.43 KB | dawehner |
| #23 | drupal-1849280-23.patch | 3.44 KB | dawehner |
| #20 | drupal-1849280-20.patch | 3.36 KB | dawehner |
Comments
Comment #1
dawehnerComment #3
dawehnerYeah this shows some bugs in defineOptions implementations.
Comment #5
dawehnerThis time with a proper default value for the boolean handler.
Comment #6
berdir#5: drupal-1849280-5.patch queued for re-testing.
Comment #8
dawehner#5: drupal-1849280-5.patch queued for re-testing.
Comment #10
dawehner#5: drupal-1849280-5.patch queued for re-testing.
Comment #12
dawehner#5: drupal-1849280-5.patch queued for re-testing.
Comment #13
dawehner#5: drupal-1849280-5.patch queued for re-testing.
Comment #15
dawehnerThis contained some other patch internally.
Comment #17
dawehner#15: drupal-1849280-15.patch queued for re-testing.
Comment #18
dawehner#15: drupal-1849280-15.patch queued for re-testing.
Comment #20
dawehnerLet's fix it.
Comment #21
dawehner#20: drupal-1849280-20.patch queued for re-testing.
Comment #23
dawehnerJust another rerole
Comment #25
tim.plunkett#23: drupal-1849280-23.patch queued for re-testing.
Comment #26
tim.plunkettThe changes look great! Two small nitpicks
Fills
No blank line needed
These are indented 2 spaces too much, and the (optional) goes before the key name I think
Hahaha, what was that? :)
Comment #27
dawehner#23: drupal-1849280-23.patch queued for re-testing.
Comment #28
dawehnerYeah indeed this patch showed some odd bugs in the code :)
Thanks for the review!
Comment #29
tim.plunkettThe 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!
Comment #30
dawehnerWe 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.
Comment #31
damiankloip commentedIf we are trimming the fat out of this method, should/could we also assume that if 'contains' is set, it will be an array.
Small thing: Should we add the translatable key to the docs too?
I have also just opened #1922966: Remove 'bool' and 'translatable' key from option definitions as a related follow up.
Comment #32
damiankloip commentedOops, must have x-posted
Comment #33
dawehnerGood points.
Comment #34
webchickYay performance!
Committed and pushed to 8.x. Thanks!