🇫🇷France @jverneaut

jverneaut → created an issue.

Thank you for this comprehensive issue summary.

I'm of the same opinion as you regarding the potential impact of this change on contrib. I actually think this fix reduces the risk of this module causing undefined behavior because we are not no longer interfering with normal pager initialization.

I just added the changes to #97 and everything is green.

I found a solution for both the pagination and memory exhaustion issue. Instead of getting $pager with $this->view->getPager();, I use $this->view->display_handler->getPlugin('pager'); which doesn't trigger the setCurrentPage and findPage calls I talked about in #20.

With the proper typings added, we still have access to the usesExposed getter which is the only method called on the $pager instance, so everything works as expected. We can then safely remove the checks introduced in this issue and let the core Views module handle the page query parameter error for us.

I've tested this fix with and without exposed pagination in an exposed filters block and everything works as expected (i.e. Drupal throwing a client error which happens anyway whether we use this module or not and should be fixed when 🐛 Protect pager against [] for the page Needs work gets merged).

This change has the added benefit of being more in line with how the surrounding sort and filter handlers are handled in the same file.

I was finally able to reproduce the memory exhaustion issue that the fix was created for. I overlooked the "Expose filter in a block" part when first trying to debug this error. After exposing the Views form with Views exposed filter blocks, I also get a 500 error.

Digging deep into the call stack, its seems the error appears after calling the findPage method on a PagerParameters service with an array like code parameter.

I found an issue for the Views module in core 🐛 Protect pager against [] for the page Needs work that tries to remedy this by adding some verifications on the page parameters. After getting the MR changes locally, the memory exhaustion issue goes away and pagination works as expected.

I'm not really sure what the best course of action is to resolve this issue. We could certainly implement the same verifications on BEF side when setting the current_page and completely override this page parameter to current_page logic, but that seems beyond the expected scope of the module. Maybe we could disable the setCurrentPage calls altogether given some specific conditions to determine.

Again, I'll be happy to help if anyone has any pointers on a proper fix implementation.

I'm reopening this issue because this fix broke pagination on one of my clients site running Drupal 10.3.2. Comparing the 7.0.X branch (which is now the recommended one) with the 6.0.x branch and looking specifically at setCurrentPage calls, I saw this issue number mentioned in the commit.

I haven't dug that deep into the issue, but it appears that $this->view->getCurrentPage() returns null at least once before being set by a view pager plugin. When null, the added !is_numeric($current_page) || $current_page < 0 check added from this issue is true and the setCurrentPage method is then called with 0 (not null), which in turn appears to break the pagination somehow.

Again, I haven't dug deep enough to figure out why this is happening on the Views side, but I'm happy to do so if needed. For the time being, simply adding a not-null check to the condition does the trick for me as per the attached patch. I wasn't able to reproduce the fatal error that started this issue without the provided fix added, so I can't say for sure that it will work after adding mine, but I also don't see how this patch could break the fix given that the $current_page is already null anyway.

This module is missing a filter settings schema definition file at the moment, here is a patch that adds a minimal one based on the settings defined in TocFilter.php to resolve this issue.

I dug a little deeper and found in the configuration schema docs → that the appropriate type for configuration objects with unknown keys is actually sequence, I then tried to create a configuration file at /config/schema/shortcode.schema.yml at the root of the base shortcode module with this content and I can then save text formats successfully and the config gets correctly exported.

filter_settings.shortcode:
  type: sequence
  sequence:
    type: boolean

Thank you for this quick resolution, I'm happy to now be part of this community.

jverneaut → created an issue.

Like #10, I'm using the shortcode module without the basic tags. After some trial and error, I was able to get the provided shortcode_example module to work by adding the following minimal configuration at shortcode_example/config/schema/shortcode_example.schema.yml where col is the name of its defined shortcode:

filter_settings.shortcode:
  type: mapping
  mapping:
    col:
      type: boolean

It is therefore possible to add configuration mappings to the base shortcode module from custom shortcode modules which was a sufficient fix in my case to be able to save the text formats.