- ๐ง๐ทBrazil mabho Rio de Janeiro, RJ
Ok, here is a suggested patch using
IntersectionObserver
. The previous approach was:- Detach the entire permissions table from DOM.
- Manipulate it to create and apply one dummy checkbox per permission, per role.
- Re-insert the entire table back to the original container.
Although detaching/re-inserting is probably an improvement over the previous version, it still causes considerable lag when loading the permissions page in the context of multiple roles and modules, where literally thousands of checkboxes are manipulated by the script.
This new approach I am suggesting is dropping the detach/re-attach method. Instead, the new code will only kick in when necessary by using IntersectionObserver; we will do the bare minimum to make sure the expected results are still delivered; here is an overview of what is being delivered:
- Grab all table rows and find those carrying a checked checkbox in the
Authenticated user
column - Immediately run the code that activates the dummy checkboxes on these rows. We want this process to happen very soon.
- For all the other rows, we have no hurry, as the dummy checkboxes will be activated upon user interaction only. Why block the entire page processing because of these guys? We will attach an IntersectionObserver to each one of these rows, and then process them individually as they become visible on the page.
I ran some local tests with
console.time()
andconsole.timeEnd()
applied to the beginning and end of script and the performance gain was massive. - ๐บ๐ธUnited States attheshow
This is a re-roll of patch #69 for Drupal 10.3.
- ๐บ๐ธUnited States attheshow
@EliasPapa Where are you seeing that the issue has been addressed? We're not seeing that anywhere in the code for 10.3.
- ๐จ๐ฆCanada Liam Morland Ontario, CA ๐จ๐ฆ
Tests do not pass. This probably needs a rebase.
- ๐บ๐ธUnited States Chris Burge
This makes sense. I have a customer who leverages paragraphs for their site, often with a dozen or more rich text fields on given node edit page. Page load times are quite slow in these instances.
The
::getAttachments()
method belongs to theDrupal\editor\Plugin\EditorManager
class, so moving this issue to editor.module.Static caching seems reasonable.
&drupal_static()
can be used. Editors and text formats have a 1:1 relationship. Text formats have permissions and control access their editor, so I don't see any permission issue here with caching.We'll want to make sure to preserve the behavior of
hook_editor_js_settings_alter()
(i.e.$this->moduleHandler->alter('editor_js_settings', $settings);
and below should not be cached. - ๐ฌ๐ทGreece vensires
Considering the fact that there are other issues related like ๐ Unable to explore tokens Needs work and that Fast Token Browser is not usable in D8 (see #3054481: Port Fast Token Browser to Drupal 8 โ ), might it be time for this issue to change to the D8 version? And if we prefer this to still exist as a separate module, then let's focus our efforts there.
- ๐ง๐ทBrazil mabho Rio de Janeiro, RJ
The patch on this thread can't be applied anymore because of the changes introduced here #3278415: Remove usages of the JavaScript ES6 build step, the build step itself, and associated dev dependencies โ .
- ๐ณ๐ฑNetherlands bbrala Netherlands
Well at least everything is green. So thats good. But it some weirdness with the event subscriber. Not sure how that should work. I didn't investigate the redirects and such. Mostly just refactored the current recent patch to work with an event subscriber.
- ๐ณ๐ฑNetherlands bbrala Netherlands
I played around with this and moved the implementation to a eventsubscriber as asked for by @znerol. I see one things that is very weird though as per comment in the code:
/** * Either Drupal\Tests\page_cache\Functional\PageCacheVaryTest::testPageCacheWithVary * fails or Drupal\Tests\language\Functional\LanguageBrowserDetectionAcceptLanguageTest::testAcceptLanguageEmptyDefault * fails if you remove one of those. Not sure why */ $events[KernelEvents::RESPONSE][] = ['onRespond', -100]; $events[KernelEvents::RESPONSE][] = ['onRespond', 100]; return $events;
Not sure why, those priorities sometimes confuse me a little.
- @bbrala opened merge request.
- ๐ณ๐ฑNetherlands bbrala Netherlands
bbrala โ changed the visibility of the branch 2430335-browser-language-detection to active.
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
- ๐ณ๐ฟNew Zealand quietone New Zealand
Hmm, we are well past Drupal 8 deployment challenges. I am closing this as outdated. If that is incorrect, add a comment and set to active.
Cheers,
- ๐ฌ๐งUnited Kingdom catch
I did think about test coverage here but all we could really do is assert the contents of the cache entry when no routes are found rather than a performance test, which feels a bit too much like repeating what the code does.
- ๐บ๐ธUnited States smustgrave
early returns make sense.
Not for this issue but would be neat to eventually have a performance test around caching if possible?