Going to update the title to add some context.
Thanks for the work on this. The original proposed solution would be one that I'd like to merge in, but I'm glad there is a working patch for folks that are running into this now.
If I'm understanding MR 8 correctly, for folks that aren't running into issues right now, this patch seems like it would have a slight negative effect as there would be new situations where saved selections would be wiped out. I didn't test this out, so maybe that impression is incorrect.
The original proposal, as I'm picturing it, would have no effect on users that use the module without issue right now while giving users that are able to generate errors an easy way out (after a failed load of the permissions page, it would return to defaulting to showing nothing with a helpful message about why it ended up in this state).
I was tasked with debugging some quirky breadcrumb issues this week, and I wasn't able to reliably replicate the problem until I came across the details in #15. With that info, I was able to get my local site into the state of having invalid breadcrumbs and can confirm that the fix in !22 has resolved my issues.
@duaelfr: Thanks for the patch and the detailed explanation. The timing of cache clears, our menu structure, and the order of pages visited meant that we hadn't been seeing this outside of production, but what you've described aligns with what I'm seeing (not related to language).
The max_input_vars is pretty easy to check against the permissions form to make sure that you aren't going to submit a form that contains too many permissions checkboxes (though the module makes some guesses about the number of non-checkbox inputs on the form). Dealing with out of memory errors would be a different beast as there wouldn't be a good way to know how many permissions would lead that error. Out of curiosity, what kind of numbers for roles/permissions and memory limits are causing this problem?
$this->keyValueExpirable->setWithExpire($this->currentUser()->id(), $values, 3600);
The selections are saved in that manner, so I don't think there is any cache clearing that helps there and as justcaldwell mentioned, it will take an hour to expire for that user.
I can see where a solution to this problem does line up with what this module is meant to help with, but I'm not sure of a good solution. Maybe something like only storing that key/value through AJAX after the form successfully renders, which doesn't prevent the error but would give a clean slate after it occurs.
Thank you for taking a look. I've patched our 1.0.11 version with the recent security fix and will test out a switch over to 1.0.x-dev (or 1.0.14 if it lands) in our next deploy cycle.
Yeah, with drush cr I get:
Fatal error: Uncaught TypeError: Drupal\acquia_dam\AssetLibraryBuilder::__construct(): Argument #6 ($theme_manager) must be of type Drupal\Core\Theme\ThemeManager, Drupal\Core\Entity\EntityTypeManager given, called in /var/www/html/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and defined in /var/www/html/docroot/modules/contrib/acquia_dam/src/AssetLibraryBuilder.php:129
Stack trace:
#0 /var/www/html/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(259): Drupal\acquia_dam\AssetLibraryBuilder->__construct()
#1 /var/www/html/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService()
#2 /var/www/html/docroot/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php(20): Drupal\Component\DependencyInjection\Container->get()
#3 /var/www/html/docroot/core/lib/Drupal/Core/Entity/EntityResolverManager.php(106): Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition()
#4 /var/www/html/docroot/core/lib/Drupal/Core/Entity/EntityResolverManager.php(219): Drupal\Core\Entity\EntityResolverManager->getControllerClass()
#5 /var/www/html/docroot/core/lib/Drupal/Core/EventSubscriber/EntityRouteAlterSubscriber.php(48): Drupal\Core\Entity\EntityResolverManager->setRouteOptions()
#6 [internal function]: Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber->onRoutingRouteAlterSetType()
#7 /var/www/html/docroot/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func()
#8 /var/www/html/docroot/core/lib/Drupal/Core/Routing/RouteBuilder.php(189): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#9 /var/www/html/docroot/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php(83): Drupal\Core\Routing\RouteBuilder->rebuild()
#10 /var/www/html/docroot/core/includes/common.inc(485): Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()
#11 /var/www/html/docroot/core/includes/utility.inc(41): drupal_flush_all_caches()
#12 /var/www/html/vendor/drush/drush/src/Commands/core/CacheRebuildCommands.php(66): drupal_rebuild()
#13 [internal function]: Drush\Commands\core\CacheRebuildCommands->rebuild()
#14 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array()
#15 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback()
#16 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter()
#17 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(391): Consolidation\AnnotatedCommand\CommandProcessor->process()
#18 /var/www/html/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute()
#19 /var/www/html/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
#20 /var/www/html/vendor/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand()
#21 /var/www/html/vendor/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun()
#22 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(110): Symfony\Component\Console\Application->run()
#23 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(40): Drush\Runtime\Runtime->doRun()
#24 /var/www/html/vendor/drush/drush/drush.php(139): Drush\Runtime\Runtime->run()
#25 /var/www/html/vendor/drush/drush/drush(4): require('...')
#26 /var/www/html/vendor/bin/drush(119): include('...')
#27 {main}
thrown in /var/www/html/docroot/modules/contrib/acquia_dam/src/AssetLibraryBuilder.php on line 129
[warning] Drush command terminated abnormally.Thanks all. I've committed logo.png and you'll see it now at the top of the project page: https://www.drupal.org/project/filter_perms → which I believe means it will show in project browser as well.
There is a 3.0.0-rc1 available now for D10, and after that gets some usage if there are no reported issues we can set a stable release.
I'm a maintainer on this module, but haven't had a use for it since Drupal 6 or 7. I'll test out this patch and merge it in today, but it would be great if a developer that is actively using and contributing to this module wanted to jump in as a co-maintainer so that this could get some better attention.
People have picked up the release candidate without issue, so I've gone ahead and tagged a 2.0.0 stable release.
The module itself is opted into security coverage, and you can see that the Drupal 7 version is covered, but like justcaldwell noted, there is a caveat of:
"Stable releases for this project are covered by the security advisory policy."
I'll go ahead and close this ticket as 🌱 What would be needed to get a stable release of the module? Needs review is handling the task of getting the D9/D10 version as a stable release.
I fixed up that multilingual multiple tab bug and agree with your assessment of stability. I created a 2.0.0-rc1 release, so if nothing changes I'll probably aim for a 2.0.0 release in November.
Thanks for the report and proposed solution. https://git.drupalcode.org/issue/filter_perms-3377610/-/compare/8.x-1.x.... from this issue's feature branch is working for me.
I ran into this as well post PHP 8 upgrade, and looking at similar issues with:
https://www.drupal.org/project/nodeorder/issues/3208010
🐛
TypeError: PDOStatement::fetchAll(): Argument #2 must be of type int, string given in PDOStatement->fetchAll()
Fixed
https://www.drupal.org/project/watchdog_prune/issues/3244788 →
I committed a simpler fix to the issue fork that is working for me.
Thank you for the patch. I was having the same issue after a php 8.1 upgrade, but for me it was causing an error during cron runs. After applying this patch, cron ran without error.
With https://github.com/laminas/laminas-servicemanager/pull/188 merged into the 4.0 branch, can this be picked back up?
@irinaz can you give the latest code a try and report back on which reports are working differently for you in the webui than they do via the drush command? i think there are a few more adjustments needed to the wrapped drush functions.
I'm working on this a bit from Drupalcamp Asheville, setting that HTML option improves the readability and I've added a small config page to more easily debug individual reports.
I had a little time tonight to push a proof of concept for wrapping all of the drush functions. The report page will now generate reports without errors, though a lot of the wrapped drush functions aren't returning values equal to the drush equivalents so those still need improved before the reports can be trusted. I also hit some issues with the best practices report that I didn't have time to solve, so I've excluded that one for now.
@devkinetic on the "minimal effort" path, I was wondering how it would look to wrap all of the drush commands that are in use in the module, so building off of how Jon had something like:
if (!function_exists('dt')) {
function dt($text) {
return t($text);
}
}
We'd also have stuff like:
if (!function_exists('drush_set_error')) {
function drush_set_error($message, $type = NULL) {
drupal_set_message($message, 'error');
}
}
And then to start we could have something like:
if (!function_exists('drush_parse_command')) {
function drush_parse_command() {
return ['command' => 'audit all'];
}
}
which could probably be altered a bit to pull settings from a configuration page and give a subset of the full audit.
Then for stuff like drush_command_invoke_all_ref(), outside of the drush environment we could wrap that, but not have any logic since it won't be applicable to the web ui.
If that's possible, we may not need to touch much (or anything) outside of the main .module file, which would mean we could be pretty sure of not breaking any of the existing drush generated reports. While this isn't a great solution, it could be a fit for the short term task of adding something to Drupal 7 in order to help get people off of Drupal 7.
Jon Pugh → credited cYu → .
Cool, thanks for working through the D10 upgrade. I’ve added you as a maintainer.
If anyone finds their way here after Drupal 7.93 and their links aren't working, the module default of "javascript: void(0);" isn't one of the special cases added and still won't work, but changing your config setting to "javascript:void(0);" by removing the space before void or using any of the following will work.
From: https://git.drupalcode.org/project/drupal/-/commit/9a82e00
$special_case_js_paths = array(
'javascript:void()',
'javascript:void();',
'javascript:void(0)',
'javascript:void(0);',
'JavaScript:Void(0)'
);