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.
Follow-up from #1846454: Add Entity query to Config entities.
This won't be a performance improvement as it stands, but if we optimize the config entity query then it'd get it for free.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 496 bytes | dawehner |
#22 | drupal-1900962-22.patch | 2.17 KB | dawehner |
#18 | drupal-1900962-18.patch | 2.16 KB | dawehner |
#15 | drupal-1900962-14.patch | 2.04 KB | dawehner |
#14 | drupal-1900962-14.patch | 2.59 KB | dawehner |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedGoing to look at this.
Comment #2
damiankloip CreditAttribution: damiankloip commentedHmm, not sure we can do this, as most of what this is used for are on display instances. E.g. getting 'uses_hook_menu', which is from the plugin definition.
Comment #3
tim.plunkettWe could try writing it out to the file and see which is more performant.
Comment #4
dawehnerJust realized that using efq could be even a performance boost.
Currently it initializes ViewExecutables for all views, but with efq you simply wouldn't have to anymore.
Comment #5
damiankloip CreditAttribution: damiankloip commentedThe prob is that currently displays are assumed to be enabled, or 'enabled' could be set to 0 or 1. I'm not sure if we can do an efq query where menu/path IS NOT NULL *and* enabled IS NOT NULL OR <> 0 ?
Comment #6
damiankloip CreditAttribution: damiankloip commentedSo we could do something along the lines of this? Then atleast we are filtering down the initial result set.
Comment #7
damiankloip CreditAttribution: damiankloip commentedOops.
Comment #8
tim.plunkettI guess we only use this for 'uses_hook_menu' now that blocks are derivatives, so this won't break, but just noting we'll have to axe the $type or do this conditionally
foreach (drupal_container()->get('plugin.manager.entity')->getStorageController('view')->load($query) as $view) {
Missing ;
Comment #9
damiankloip CreditAttribution: damiankloip commentedComment #10
damiankloip CreditAttribution: damiankloip commentedThanks Tim. Generally it's a good idea if I actually check patches first ;) So we should use the $type param and change it it path for now maybe. Also changed your other points.
Comment #11
tim.plunkettSo this assumes that if one display has a path, then all displays of that view have a path. Here's an ugly hack for that, but we'll need a better solution.
Comment #12
tim.plunkettHere's another option, to preserve the existing behavior and prevent an API change.
Comment #13
damiankloip CreditAttribution: damiankloip commentedYeah, I guess the only trouble with that is that disabled views would be returned too, unless you always passed in the ids..
Comment #14
dawehnerWhat about that?
Comment #15
dawehnerLet's edit the patch file :)
Comment #16
damiankloip CreditAttribution: damiankloip commentedI like that.
Comment #17
damiankloip CreditAttribution: damiankloip commentedLet's just use FALSE here instead. Then we're consitent with the patch in #1901076: Use config query to get enabled and disabled views. too.
Comment #18
dawehnerNew version.
Comment #19
tim.plunkettConsidering that my patches weren't used, I feel comfortable RTBCing this. (If it comes back green)
Comment #20
damiankloip CreditAttribution: damiankloip commentedThat looks like we're all happy with this now. Nice team work.
Comment #22
dawehnerDOH
Comment #23
tim.plunkettHah, nice catch. I manually tested this time and it works. That interdiff was the only problem.
Comment #24
chx CreditAttribution: chx commented->condition("display.*.display_plugin", $ids, 'IN')
. WIN.Comment #25
webchickI know catch said he was somewhat concerned about this getting abused everywhere, so I was going to assign it to him, then noticed that he created the issue in the first place. :D
Committed and pushed to 8.x. Thanks!