🇧🇷Brazil @mabho

This error is showing up to me when I set something that Drupal probably sees as an invalid value.

In order to reproduce it, please try this:

• Navigate to /admin/structure/events/series/settings
• Update the value of Event Series Minimum Time to something in the 0 hour range (e.g.: 00:00am or 00:30am).
• Save the settings form.
• Navigate to /events/add/default.
• Check the WSOD being displayed with the exact message on this thread.

It appears 0:00am is not a valid format; apparently I should use 12:00am instead. I changed the configuration on /admin/structure/events/series/settings to 12:00am and it resumed working, now the starting time on the range is midnight (0 hour). Ok, I wonder if there should be a form validation to confirm the date entered by the user in Event Series Minimum Time is a valid format to be processed by Drupal\Component\Datetime\DateTimePlus::createFromFormat() or not.

Actually, the problem still occurs for me. But it is intermittent, and I can't determine the cause.

The fix I mentioned on #90 was helpful, it dramatically decreased its frequency, but the problem is it still happens.

The same page might fail in production, and still work in DEV or STAGE, which makes things very hard to debug.

I will post here if I find a solution, or if I can get a new insight for a solution.

I am currently experiencing the exact same issue @mibstar mentioned above (#88) on a website. External files hosted in Adobe Fonts' Typekit and Google Fonts were triggering the problem.

fonts:
  css:
    theme:
      https://use.typekit.net/[css-file-path]:
        type: external
      https://fonts.googleapis.com/css2[font-params]:
        type: external

I moved the references to these external assets into @import statements within the CSS and swerved the problem without actually resolving the underlying cause.

The fact that the <figure /> tag is showing within CKEditor5, and not outside it (when the image is rendered on the page) seems to be the problem to me. My take on this is: when the image is rendered, no matter what, the <figure /> wrapper should be applied. I wonder what would be the unintended consequences of such a change on legacy websites...

My 5 cents here. You have done a great job, and clearly reached a milestone with the new code: does it deliver everything the previous equivalent, scattered filter versions were doing? The answer, as I see it, is a sounding ‘yes’.

But there is much more. Let’s see:

• It is preventing redundancy (multiple modules implementing the same thing) with different approaches and methods. It thus delivers consistency.
• Users will be loading a single Javascript file for this functionality instead of multiple Javascript files (one per module). Call it a marginal improvement, but we are preventing many Javascript files from being downloaded from the server because users now depend upon a single library for that functionality.
• The new implementation is lean and fast, probably better than all the implementations on individual modules. It saves users' resources, and, considering Drupal's scale, it will help save some trees as well.
• Module developers willing to use a filter can use this approach instead of developing yet another custom implementation.

I would advocate to move this thing forward immediately, and write new individual tickets to improve the code (grouping with children DOM elements; comprehensive API to help module developers extending the code) after it has been merged.

This ticket touches several different files. The more it is delayed, greater are the chances that breaking changes on individual files will require us to analyze the code again and fix conflicts, thus consuming precious time, and delaying things even further.

Quoting Stevie Wonder, this is 'Signed, Sealed, Delivered', what should be the next steps?

mabho → created an issue.

@joachim, thank you for clarifying.

From what I have seen, I would assume group is an element that contains the children, e.g. details element, or a DIV is the remaining structure that needs to be tackled/resolved, right? It seems to me the current version is implementing the grouping functionality via group is an element that precedes the children, e.g. a table TR.

Do any of the Drupal core structures using the filtering library rely on group is an element that contains the children, e.g. details element, or a DIV, not covered on the current code?

My question would be: do we really need to support/handle the parent/child approach for any of the core functionality using filters?

mabho → created an issue.

Guys, considering the latest state of the code put forth by @finnsky, what is the filter structure (user permissions, views, modules list, fields etc.) that would require the grouping functionality mentioned above? I tested the latest version of the code on the permissions filter (/admin/people/permissions), and it seems to work alright: only titles associated with the found permission rows are activated/displayed.

@chris burge: yes, I did clear cache, but the code is still broken after that.

mabho → created an issue.

Sure thing, @aaronchristian, I am happy you agree with the solution, and that I could contribute back to the module's codebase :-)

After applying the DIFF made available here (https://git.drupalcode.org/project/oembed_providers/-/merge_requests/25....), I get this exception message:

The website encountered an unexpected error. Try again later.

Error: Class "Drupal\oembed_providers\Helper" not found in oembed_providers_form_media_type_edit_form_alter() (line 61 of modules/contrib/oembed_providers/oembed_providers.module).
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'media_type_edit_form') (Line: 834)
Drupal\Core\Form\FormBuilder->prepareForm('media_type_edit_form', Array, Object) (Line: 285)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object) (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 741)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Here is a static patch file out of the changes delivered in commit 2a5f14b79fd68d6c2d92c96eaffad63728558d0a.

mabho → created an issue.

@finnsky, @oachim: I reran the failed functional test from the UI on this page (https://git.drupalcode.org/issue/drupal-3028968/-/pipelines/284678) and all went fine with no failed tests (https://git.drupalcode.org/issue/drupal-3028968/-/jobs/2775475). I wonder if a wait() or sleep() would be recommended at some point in the test script to prevent an intermittent, false-negative result. I also ran the tests locally, and didn't bump into any problems. I assume the script is thus passing the tests.

I am looking forward to your comments/reviews, and let me know if you have any questions, thank you!

Thank you for sharing that information, @finnsky, I will try these commands.

Hum, I see the code fails the pipeline, I guess I need to run Javascript code linting against the Drupal recommended standards, I will check how to do that...

Guys, I just pushed the code. This is how I suggest testing this:

Add console.time('timer') and console.timeEnd('timer') to measure the time it takes to execute the event attached to the table instance. I suggest applying the same change to the original code for comparison. It can be applied exactly at this position:

      e.target.addEventListener(
        'input',
        debounce((event) => {
          tables.forEach((tableElement) => {
            console.time('timer')
            tableElement.dispatchEvent(
              new CustomEvent(FILTER_EVENT, {
                detail: {
                  event,
                },
              }),
            );
            console.timeEnd('timer')
          });
        }, 200),
      );

This performance test will measure the time it takes to run each keystroke. It won't take into account the time we are saving by not activating the filter script on each page load.

Let me know your thoughts.

I moved my activity into issue thread ✨ Create Javascript library for searching rendered lists on the client. Needs work because it is proposing a single library to be used everywhere a filter is needed. It promotes code reuse and provides a pluggable mechanism that can more easily be reused.

@finnsky, I am getting a message stating that I am. not allowed to push code to this project. Is this something you can grant to me?

git push origin 3028968-
remote: 
remote: ========================================================================
remote: 
remote: You are not allowed to push code to this project.
remote: 
remote: ========================================================================
remote: 
fatal: Could not read from remote repository.

Guys, I went over the frontend part of the 'problem', and started off from @innsky's code, which doesn't mean I have any opinion on the ideal approach discussed above. I think the ideal place is somewhere in the middle: keep the backend API allowing module/core developers to provide customization and override the defaults, but provide a set of conventions that would allow users to quickly plug the filter library by using the defaults (table selector, targets, etc.); this way, if no complexity is required, someone could easily and quickly plug the library and have the filters working immediately if no customization if a more complex/custom setup is not needed.

That said, here is the explanation on the changes I applied on the code available in branch 3028968-:

• Use a case-insensitive (this was working already), accent insensitive search (Latin languages use accent very often; I can tell you it is very frustrating when systems don't take accents into account; I am thus 'normalizing' strings before searching
• Instead of searching straight into the DOM elements themselves, I am setting up an array of objects to iterate through. Each array item object carries a lower-case, no-accent version of the string being searched for, as well as a reference to the corresponding DOM element being manipulated.
• I am setting a mechanism to accelerate the queries. Imagine 200 rows to search from. Every time you type a letter, the script searches among the entire 200 rows. Usually, though, users are incrementally typing a word, of multiple search words, and we don't actually need to run a lookup on the entire 200 rows for each keystroke. Example: type string 'con' for 'content' and you are already ruling out most of the potential rows not carrying that string sequence. The next time the script is triggered (let's say, when you completed typing 'conte'), there is no need to start from the 200 records, but we can use a temporary resultset including the ~ 20 search results for 'con'; this tends to deliver good performance improvement on larger tables (not so much on smaller sets, but it still improves the speed).
• The approach with once() is indicating that we can potentially carry more than one input filter per page. If that happens, we don't want all the variables to be duplicated (one instance per input/table). I separated that portion of the code to prevent that type of problem and keep resources usage lower in that case.
• Last but not least, probably the most important change: we don't need to activate the filters if users are not using it; I would say, most of the times users don't use the filter when reaching the users permissions page, or modules list page. I am thus keeping the filter dormant until the filter field is focused. When that happens, then the entire mechanism is activated.

The code is ready on my local environment. @finnsky, let me know if I can push it over your branch as it applies many changes to the original code (the original concept and structure is preserved). Feel free to review, comment and improve from there, and of course, if you don't like it, it is always possible to revert my commits.

Hello, guys, do you want me to go over the code and try to remove jQuery, switching it to a pure Vanilla JS approach?

On this ticket 📌 Improve the performance of the script delivering the permissions search filter functionality in admin/people/permissions. Needs review I was able to achieve an excellent performance enhancement (affecting the user permissions page only) by getting rid of jQuery on the user permissions script (core/modules/user/user.permissions.js). The code changes are available here: https://git.drupalcode.org/project/drupal/-/merge_requests/9433/diffs.

I am putting my thread on hold since this issue creating a Drupal filtering library is a much more promising approach.

If I can help adjust the code to a no-jQuery approach, should I commit and submit the changes on the main branch? Or on a separate branch? Let me know what would be your recommendation.

My only concern about this permissions table is its mechanism to hide/show the parent of the found record (in this case, the module name associated with the found permission). I wonder if the implementation being carried out here (https://git.drupalcode.org/project/drupal/-/merge_requests/6309/diffs#58...) is taking care of this. This is speculation, though, and I need to look into that implementation.

Thank you @_nod and @nicxvan. I can see that initiative is pretty advanced and removes the dependency on jQuery... Maybe the suggested approach here (taking advantage of a subset of the records being searched based on the text previously typed) can be ported into that script for improved performance... or not. I will take a look at it.

Real-time commenting and adjusting ;-) Just fixed the typo, many thanks for the quick feedback!

I am introducing some enhancements with the purpose of making the code run faster.

First things first, removing jQuery delivers relevant performance enhancement. It is amazing how slow the code can get on some operations triggered by jQuery. This becomes more dramatic on a script being activate on each keystroke by the client.

Apart from that, the code is also introducing transitional variables storing information on the most recent text typed and sample search results extracted. By comparing the previous search string with the current, the program can determine if the user is incrementally typing a word or phrase. If so, it will incrementally run the new row filtering from the sample results on the previous round, instead of doing it from scratch every time.

This dramatically reduces the number of rows the program needs to analyze, thus improving performance.

A merge request is already available.

mabho → created an issue.

I am providing an updated patch file.

I have just created a merge request for the changes in this thread. I am also uploading a patch file for those who prefer this method.

I am using this patch on version 2.1 (it still works there). I have thus changed the version of the module being tested against to 2.x-dev. I hope this is not a problem.

Ignore this issue, the problem I am reporting here is being introduced by custom code on issue ✨ Add a weight to the processors, so we can sort when they're being executed Needs work .

mabho → created an issue.

@_nod, the changes you are introducing are brilliant, they improve the code even further. My only stylistic comment has to do with the name of the IntersectionObserver variable. Since we are now observing checkboxes, not table rows, I think the variable should be renamed accordingly.

Thank you @_nod and @joaopauloc.dev for the updates and tests on this ticket, I am happy to see my initial effort seems fruitful. I can't test the latest changes by @_nod right now, but will try to do so next week.

pameeela → credited mabho → .

mabho → created an issue.

Here is an updated version of patch #03 that should apply to the latest version of 8.x-3.x-dev. It is also compatible with the latest patch in 🐛 Marker-Scroll-To-ID throws "TypeError: marker.addListener is not a function" Needs review .

... and here is a patch file corresponding to the changes suggested in the merge request.

I have just submitted a patch to fix the issue discussed on this thread.

mabho → made their first commit to this issue’s fork.

Updated the issue queue description to reflect the latest changes I had applied to simplify the code even further.

What is missing to get the patch by `@kushbatt` (#13) applied to the codebase? His fix does work, and the bug is still out there...

I am sorry, this issue had already been addressed here ( https://www.drupal.org/project/gin_lb/issues/3426166 💬 Update requirements Fixed ), and the patch that fixes it is this DIFF file: https://git.drupalcode.org/project/gin_lb/-/merge_requests/86.diff. Unfortunately, it hasn't been merged yet, possibly because the ticket is already set as Closed (fixed), but the most important patch/merge request there hasn't been applied yet.

mabho → created an issue.

mabho → created an issue.

@abhiyanshu, I can see your changes on top of mine, thank you for your contributions, it truly helped me see further and investigate deeper on this validation landscape, I am learning a lot from this interaction.

Some comments here:

• We don't need to validate anything within the fonts/* folder. That folder is auto-generated by Icomoon and I don't want to revalidate things every time I push a new icon (when that happens, Icomoon generates an entirely new package of files that I just replace/overwrite on top of the existing ones, including CSS, HTML, SASS, font files). We can exclude any changes within that folder.
• The updates need to be applied to the SASS files, we don't want to validate the compiled CSS files. We also want folder /js/dist to be left out of the validation.
• It looks like Javascript code validation is done via Eslint, not PHPCS. Please, check this documentation on how to specify Drupal coding standards ( https://www.drupal.org/drupalorg/docs/drupal-ci/using-coderphpcs-in-drup... → ) and this Acquia recommended project as references: https://github.com/acquia/drupal-recommended-project/blob/master/phpcs.x.... Javascript should thus be left out of the <arg name="extensions" /> values.
• I am also leaving the .md file out of the PHPCS scope, I reworked the file to limit each line to 80 characters maximum, but it is very limiting to keep that rule.
• Finally, I have an update for the phpcs.xml.dist file that is redeclaring the exclude folders within the exclude-pattern rules block and, most important, it is adjusting the file extensions we want to validate according to the benchmarks mentioned above. In order to run PHPCS out of these new rules you will need to rename phpcs.xml.dist to phpcs.xml. You can run it with the command you see in the documentation: vendor/bin/phpcs --standard=web/modules/custom/visual_debugger/phpcs.xml web/modules/custom/visual_debugger (edit paths accordingly depending on your environment).

Because of all these differences, I reverted your commit, and packed the adjusted phpcs.xml.dist file, but your help was invaluable and I will provide all the credit on this thread.

One final question: do you know if we can get PHPCS to validate SCSS files with the same recommendations it delivers to CSS files: my SCSS validation did not find the problem with the hexadecimal uppercase codes and other issues the original validation raised when running on CSS.

requestAnimationFrame will 'schedule' the repaint associated with activating/deactivating the checkboxes to the next repaint.

@nicxvan: when the permissions page is loaded, I am immediately running the script to activate the dummy checkboxes on the checked columns of Authenticated user.

This will for sure cause the script to slow down the page in a situation where you are checking all checkboxes in the Authenticated user column because the code will run more or less like the previous version where all rows were being processed upon page load.

In my experience, though, that is something we don't see in real life (authenticated users being granted most of the permissions).

You bring up a point that makes me think further: why should I replace those checkboxes when the page is loaded? Why not simplify the script even further and let IntersectionObserver do the magic for all rows, including the pre-checked rows?

I think I will remove this preprocessing of the authenticated rows, and use a one-rule-fits-all using IntersectionObserver. The further performance gain in this case will be minimal, but the code will be even leaner.

A comma-separated list would be much worse in terms of readability. If you want to get a better experience inspecting theme suggestions than scanning through the source code, I can recommend the Visual Debugger module → .

I got some feedback on the Drupal Slack channel from some Drupal community colleagues who thought my proposed resolution would suit better in a new, separate ticket. I have thus created an issue here ( 📌 Improve performance of the user.permissions.js script running in /admin/people/permissions. Needs review ) with a new merge request carrying some improvements over the patch file posted on this issue queue.

I just attached screenshots of my Firefox browser running the project I mentioned with the Javascript performance audit script, both on the original code (403ms) and the new code (12ms).

mabho → created an issue.

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() and console.timeEnd() applied to the beginning and end of script and the performance gain was massive.

@abhiyanshu, I just completed applying multiple changes to the project, and released a new alpha9 version. If you are still willing to run the task on this issue queue, you can grab the latest version just released, run phpcs again and apply the suggested fixes. Let me know if you are willing to do that. I can also go over this task. Many thanks!

Besides the automated update performed by the automated tool, there was a need to fix the code in src/Form/SettingsForm.php because of this error message being thrown in Drupal 11:

ArgumentCountError: Too few arguments to function Drupal\Core\Form\ConfigFormBase::__construct(), 1 passed in /var/www/web/modules/custom/visual_debugger/src/Form/SettingsForm.php on line 31 and exactly 2 expected in Drupal\Core\Form\ConfigFormBase->__construct() (line 43 of /var/www/web/core/lib/Drupal/Core/Form/ConfigFormBase.php).

I applied an update to fix this problem. There was a need to incorporate variable Drupal\Core\Config\TypedConfigManagerInterface to the constructor method of ConfigFormBase.

mabho → made their first commit to this issue’s fork.

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 → .

I wonder if we can approach the problem raised here using Intersection Observer... It seems like the best approach... I am suffering from a huge lag on 15 roles loading in parallel.

Thank you for taking over the ticket, abhiyanshu. I will ask you to wait for my sign to fork this and start working. Many thanks for helping!

Thank you for reporting this, Suthat, I will run PHPCS and apply the suggested fixes.

I will keep this ticket as a 'to do' for now: I have ongoing changes already affecting another branch that would trigger many conflicts if I were to work on this topic now. Once those changes are merged back to the dev branch and deployed, I can kick-start the PHPCS adjustments.

This has been fixed and tested.

mabho → created an issue.