Thanks for testing the patch and for your patience. Unfortunately, due to the many runtime exceptions for deprecated code in Drupal Core, Upgrade Status / project upgrade bot can't detect all issues and modules must always be tested manually in addition to testing with automated tools on all supported Core versions. In this case, there was one more deprecation that resulted in the config form showing a white page in Drupal 11.
I rebased the MR and fixed the config form. With those fixes, tests also pass on Drupal 11. The fix for the config form raised the minimum supported minor Core version to 10.2, dropping support for 8.9, 9 and 10.1, which are all EOL by now, so we don't need to provide a BC layer. The new pipeline also found more deprecated code to be removed in Drupal 12 that we can already replace now without further limiting Core compatibility, so I did that proactively.
Going to merge this now.
Merged. Commit URL: https://git.drupalcode.org/project/html_tag_usage/-/commit/d2ae7e6faeed6...
Thanks. I agree that we should ideally just display those tags and attributes and deal with validation separately. There is already an issue to implement that.
The MR looked good in general. I rebased and extended the tests to cover this issue. I noticed that the original MR didn't allow underscores in tags, although the original issue report was about a tag with an underscore, so I added that.
Cspell fixed now, as I said, rest of the results are as expected.
Test results as expected for now, but cspell complains about all the example HTML in the tests...
Just ran into this once more and the MR fixes it nicely, thanks for working on this.
Actually, I agree with #39. We don't provide an API to disable the new behaviour, so I think just "adapting the code" isn't a solution. I have removed this paragraph from the CR. The whole CR is more like a heads-up for people who might possibly rely on this behavior so that they can look into how to best deal with removal of this bug before updating. I don't think we actually need to provide a "solution" to restore the buggy behavior in the CR. "There is no replacement"-CRs are also not that uncommon, so it should be fine that way for this obvious bug.
Likewise, I don't think we should add an API to restore the old behavior and then provide a BC layer (most likely including UI so that you could enable it somehow) just to remove all of it again in Drupal 13. I don't think the case in #36 is common enough to warrant this and @catch didn't ask for it.
I added a release note to the IS and also added a sentence on why we think a release note might be a good idea.
Opting in for any or all of the following modules:
- https://www.drupal.org/project/html_tag_usage → (10 issues)
- https://www.drupal.org/project/language_switcher_menu → (16 issues)
- https://www.drupal.org/project/menu_entity_index → (45 issues)
- https://www.drupal.org/project/patchinfo → (30 issues)
- https://www.drupal.org/project/pathalias_extend → (13 issues)
And if you'd like to test an edge-case, there's also https://www.drupal.org/project/audit_report → (0 issues).
Some things I just did:
- Rebased to latest 8.x-1.x-dev.
- Made a few minor adjustments to the type hints/comments mostly to match my personal taste. In particular I'm not a big fan of final and private except for very rare exceptions and this is not one of them.
- Added test coverage for the Drush command. The test coverage actually found a minor bug, where the menus were not "untracked" before being "retracked". I added that part. If this was an intentional decision (e.g. for less disruptions on the live site while running the command), I'm open to add an option to skip untracking, but I think deleting existing entries before adding the new ones should be the default, because otherwise you could end up with a corrupted index unless you really know what you're doing.
- Added a
@todoto change TrackerInterface and simplify the code in 2.x. As you probably noticed, I don't have a lot of time for upstreaming contributions right now and that's why I decided to delay 2.x a bit longer and stick with 8.x-1.x for now. At some point, we will add this, maybe not even too far in the future. We can get rid of the module file, use a batch builder and more. But not just yet.
Normally, I'd wait to get this retested and reviewed by another contributor, especially for the untracking part, but I'm a bit in a hurry to get a new release out now, so I'll make an exception. With the new test coverage, I'm fairly certain that it works as intended. If you notice any issues, please file a new issue. If you want the option to skip untracking, please file a new issue as well. Thanks again for your contribution and sorry for the delay.
Thanks everyone for testing this and for your patience. The new test coverage and my own manual testing confirms your findings, so I'm going to merge this now.
Pipeline looks as expected and phpstan even found a bug in our view filter dependencies. Going to merge now to proceed with testing on D11.
Pushed two more commits to fix tests. Sorry about that, should have noticed it earlier. The remaining phpstan errors still look unrelated.
feyp → created an issue.
Looks like there is still an unresolved thread on the MR.
Thanks @catch for looking at this! I think the open thread you're referring to is from an earlier review I did? This thread had been addressed by the time I wrote #60, also confirmed by another review by @quietone in #61. I would have resolved it at the time, but even though I have push access and I am the original author of the thread, I didn't have permission to resolve the thread and I still don't have permission to do so, unfortunately. I know this is kind of confusing, but imo the thread can be considered resolved.
Setting back to RTBC per #66.
Created an MR.
Where is the current policy documented?
As already stated in the motivation section of the IS, this is currently documented at https://www.drupal.org/drupal-security-team/security-team-procedures/dis... →
I created a quick MR using GitLab's code editor.
Updated IS. Code in the MR looks good to me and fixes the problem. Alternatively, #14 provides the changes from the MR as a patch file. The MR pipeline has warnings, but it looks like those were not newly introduced by the suggested changes. We already have a bunch of users in previous comments confirming this approach fixes the error for them. I'm going to go ahead an RTBC this.
Thanks for taking a look at this longstanding issue. We have since added a Gitlab pipeline and would require this to run so that we can see if the tests pass. So the MR needs a rebase to the latest code base before it can go into review again. Setting to Needs work for that. Note for anyone who wants to take a look at this: Please do not merge, but rebase! There is good documentation on drupal.org on how to do this, if you are not sure. Thanks again.
Thanks for your offer. I see no previous involvement in the issue queue from you besides a comment on the D11 issue just a few minutes before you filed this issue. The actual heavy lifting on the D11 issue was done by other contributors. I'm currently in the process of testing the MR on a bunch of different systems, different Drupal and PHP versions and I'm actually ready to release very shortly. If you need D11 support now, you can just use the patch from the MR until then. So I appreciate your additional testing and confirmation, that everything works for you, but your contribution history to this module is unfortunately not a good basis to apply for co-maintainership, so I'm going to close this as wont fix for now.
Alright, separate module is not gonna happen, so let's close this won't fix.
Thanks for your offer.
- I don't see any previous involvement from you in the issue queue of this module.
- You are a member on d.o for 2 weeks and 4 hours.
- Your account is not confirmed.
- There is no track record of other modules you maintain or even a lot of issue credits where I could look at your past contributions.
At this time, you simply don't have enough of a history for me to even start to evaluate, if you would be a good co-maintainer of the module.
Also, I will take care of the Drupal 11 release (and other open issues) myself very soon. If I'd need any help, there'd have a number of people that I already know and trust and are already active in the community that I could add as co-maintainers.
So sadly, at this moment, I have to firmly decline this offer.
I suggest you start contributing to modules you use and maybe to core as well for a few years and maybe publish a few modules yourself before you start to offer maintaining an existing module with a larger user base. With this public track record of your contributions, it is basically impossible to evaluate, if you would be a good maintainer, even if you are already using Drupal for many years and have a lot of experience, it is just not visible through your d.o account.
Okay, so the MR has the automated test, which demonstrates the core issue, not the TAF issue. In the pipeline for previous major (D10) you'll get this:
Drupal\Tests\taxonomy_access_fix\Functional\TermReferenceFieldAccessTest::testTermReferenceFieldAccess
Field values as expected for authenticated user
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
0 => Array &1 (
'target_id' => '1'
)
+ 1 => Array &2 (
+ 'target_id' => '2'
+ )
)
To test for the TAF issue instead, uncomment the module in the $modules property to install TAF as well and both terms will be gone. Optionally set the status of the second term to published, but it won't make a difference. Only the permission to assign to fix the issue will be different. Uncomment the select (any) terms permission for the authenticated user and the test will pass.
Setting to needs work for filing a core issue and then either postpone on that one and wait for core or find a good fix in TAF.
Looked at this again after way too long, sorry for that.
I started by writing an automated test based on #3. Some things I noticed on the way:
- You don't need to implement
hook_entity_field_access()for this issue. The issue happens as soon as you enable Taxonomy Access Fix as long as the non-admin user doesn't have permission to select terms from the vocabulary. It makes no difference at all whether the hook is in use or not. - It happens in core as well. If Taxonomy Access Fix is disabled and one of the terms you assign as an admin user is unpublished, once the non-admin user saves their profile, the previously assigned unpublished term is gone. Again, it happens regardless of whether the hook is in use. This is not actually surprising, since our implementation is based on the implementation in core, we just check for select access instead of term status.
I'm not yet sure what would be the best way to fix this. Since it doesn't make a difference, if the non-admin user has permission to edit the field, I guess the approach used in the patch is not yet the final fix.
Thanks again for filing this issue and starting to work on an MR. Given that this hasn't seen any more comments/engagement from the community after my comment from approx. two years ago, I'll go ahead and close this as "Works as designed" now.
I'm still open to revisit this for a future new major release, if there is significant support for this change from the community, but as it looks like right now from the comments/followers of this issue, it seems that most users of the module are happy with the current implementation.
Unfortunately, this caused a regression for me that I just noticed, because those temporary files used up almost all disk space on the server by now for a regular import using migrate. See 🐛 [Regression] Clean up temporary files created by XML data parser plugin Active .
Pushed a quick and dirty fix where the file is deleted in a destructor to the MR.
Thanks.
Only upgrade status is still showing a warning because we don't support Drupal 12 yet.
This is expected, the job tests compatibility with D12 now. It can be ignored.
We want to keep this automated issue open until we're compatible. This should probably also be the canonical issue for D11 compatibility, because it is the automated issue, I just didn't get around to close the other one as a duplicate yet.
W/r/t plans, I need to take care of two other issues in the queue or come to the conclusion that they won't be a factor until it makes sense to really look into this. Then I'll appreciate any help with testing the MR and upgrade path.
Right now it looks like the other issue has the better MR and more community engagement, so if you need a patch now, try that one. I'll take care of merging everything into one issue, including any credit, if needed, later.
breidert → credited feyp → .
Thanks for working on this so quickly!
Tested the MR in
- https://git.drupalcode.org/project/language_switcher_menu/-/pipelines/30...
- https://git.drupalcode.org/project/language_switcher_menu/-/merge_reques...
-
https://www.drupal.org/project/language_switcher_menu/issues/3479891
📌
[DO NOT COMMIT] GitLab CI MR testing issue
Active
and now the job runs properly and issues a warning, because the core compatibility in the info.yml needs to be updated. I think this is the expected output for the job with the current code base of the module.
Looking at the changes in the MR, it looks fine to me, so RTBC from my side. Not going to mark RTBC, because I'm not sure I'm familiar enough with the CI templates and possible problems with this approach. I think we're good to go, but I might be missing something. So leaving at NR for now.
This is still an issue in 8.x-4.x.
Added STR to the IS, removing tag.
When I configured the processor to use processor_settings.glossary.grouping_defaults.grouping_other, I got variable type is integer but applied schema class is Drupal\Core\TypedData\Plugin\DataType\StringData for the other two options. So it looks like this still needs to be tweaked a bit. Setting to Needs work, also to possibly convert to an MR?
Note that there are ways to test the config schema automatically using GitLab CI, if you are okay with using config_inspector as a dependency. For now, leaving "Needs manual testing" tag in place.
Thanks Stephen, the changes look good to me.
Looks like the CI runners are a little busy right now, the repeated failures due to GitHub timeouts and no test nodes available gave me sufficient time to also retest this locally. Almost like Drupal CI, oh the good old times, where I could work on another issue while I waited for the tests to complete... Let's call it the DrupalCon review bonus :)
Test only job fails as expected:
Configuration: /builds/issue/drupal-3143617/core/phpunit.xml.dist
..EE.......EE.FF....FEE..F... 29 / 29 (100%)
Time: 00:00.034, Memory: 8.00 MB
There were 6 errors:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
2) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
5) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
6) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
--
There were 4 failures:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a string" ('0', '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 0 => 0
)
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
2) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 0 => 0
)
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "defensive null page value" (null, '', [])
Failed asserting that null is identical to ''.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that 0 is identical to '0'.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
ERRORS!
Tests: 29, Assertions: 33, Errors: 6, Failures: 4.
Exiting with EXIT_CODE=2
The documentation for the data provider return value could always be a bit more verbose of course, but I think documenting that the keys returned by the data provider correspond to the variable names that are also used in the test methods should be enough for developers looking at this test code in the future to get the idea behind this setup. It should also address @quietone's concern:
For whatever reason, I started reading this from the provider and kept wondering why there were three return values when testGetPagerParameter() is only using one.
So let's give the RTBC queue triage team another shot at this... I think this issue is RTBC 🎉!
Added testing for all supported core versions and fixed the pipeline except for next-major, which is currently still Drupal 11 and which we'll deal with over in 📌 Add compatibility with Drupal 11 Needs work . No changes except for cspell in README, phpcs and some deprecations in tests, so straight to RTBC.
That said, I’d be happy with D, C or A. Anything but B. 😂
Same here, preference now for D, so thanks for adding that :). My comment was triggered by comment #18 really, which stated that the consensus seemed to be option B. Until then, I was just a quiet follower ;).
Looking at commits in GitLab, it would be great if I didn't have to expand the commit message to get to the most important information.
I like that authors and reviewers are moved into the description and that the information is presented in a more structured manner. This information is valuable to have, because it gives credit where credit is due, but it is not the information I'm immediately interested in when looking at commits.
I'd like to have the following information on the first line:
- The issue number
- A meaningful commit summary
The issue number should be in the first part of the first line. That way, I can click right through to the issue without having to expand the commit message.
Ideally, the commit summary should start with an imperative verb in upper case (e.g. Add xyz, Remove xyz, Convert xyz).
Honestly, I don't care at all about the conventional commit stuff, I don't need that. It just takes away valuable space on the first line in the part of the commit message that is immediately visible without expanding the message and is hard to correct later, if there was a mistake and it is wrong for some reason (e.g. a change not correctly identified as a breaking change on commit). It means we have to be even more careful checking issue metadata before marking an issue as RTBC, right now we often don't even manage to adjust the issue title into a meaningful commit summary. And I really don't like the "all lower-case" commit summary line in conventional commits, although that doesn't seem to be part of the standard, it is just in all the examples?
W/r/t #40, the issue closing pattern in GitLab is just a regular expression that can be configured. The default issue closing pattern matches a whole bunch of key words already, but none of the proposals will auto-close issues in GitLab by default. B) is close to what would be needed, but won't match because of the colon. But I'm also not sure that we really want auto-closing for issues.
Looking at the proposals, A) and C) match some of my requirements. B) does not.
Looks like we already have enough supporters, but fwiw +1 on this one.
@caesius This fix has already been released today in 10.2.8, 10.3.4 and 11.0.3.
@khiminrm As a temporary solution for 10.2, you can use a cross-alias as shown by @longwave in #13. Note that you should remove the cross-alias again when updating to the official release, which will hopefully come soon.
Okay, answering my own question here, I guess it is about the autowiring part. I can see how creating a service is then easier, if don't have an existing implementation of a "hook class" already. Probably also more fool proof to ensure that the autowiring actually works later even without a create method. I just shouldn't post to issues this early in the morning... Rock on and sorry for the noise.
Reading through the CR, I wonder why we recommend creating a service for compatibility with older Drupal versions. What prevents usage of \Drupal::classResolver(), which is already the recommended way to call OOP implementations from hooks and does not require creating a service for each hook (class), thus providing better DX?
Thanks for working on this.
Regarding @quietone's outstanding question in #56:
The changes to getPagerQuery and getPagerParameter are changing the behavior, making it conform to the interface. So, it there any ramification for contrib/custom?
I think this question has already been answered by @neclimdul in #28 6):
Fixing it to match the interface _could_ break contrib or sites hacking strings or floats through a pager value. However those cases would be broken in findPage which does the cast already so it was probably already effectively broken.
The code changes look good. I added one suggestion and a few comments inline, mostly concerning comments in the test.
The MR introduces new test coverage for the pager parameter. The pipeline is green. The tests fail locally as expected without the fix. I'll run the test only job just before I'm otherwise ready to RTBC, so not quite yet. With the MR applied, I can't reproduce the original issue. I also did some manual testing both with the original and modified (invalid) pager parameters: The administrative content view (using AJAX), a regular view (without AJAX), multiple views with different pager ids on the same page, the administrative URL Alias overview page (no view, probably entity query pager?). I noticed an issue with 3 views on the same page, but re-testing this without the MR applied, the issue was already present before, so no regression.
Tagging security improvement (for contrib) based on #57.
So overall this looks good and almost ready. Ready to RTBC once my very minor MR comments have been addressed.
Thanks for creating the issue. I can already say that the change of the main info.yml is not enough. Just run the tests once and you should see the problem. But we'll need a rebase anyway once I have enabled GitLab CI. I'm setting this to needs work for now for the rest of the required changes, but it is not a good time to work on it for now.
Also, the code changes are really straight forward and mostly covered by automated tools. What I could use help with is manual testing on all supported core versions once the changes are ready. Especially if you already use the module on D10 and would be willing to test an update to D11. But again: Soon, but not yet.
Thanks. Looks good. I tested various exposed pager stuff with the MR and as far as I can see everything still works as before. I noticed an issue with the links plugin: If the exposed form is shown in a block and the block is not placed on the same page as the view, the links point to the current page instead of to the path of the view display. Luckily, this also happens without the MR, so nothing new. I'll go ahead and RTBC this.
Thanks @jverneaut, the patch looks very promising.
I tested the patch on the dev system of one of the systems affected by the regression and the pager works as expected. On my 10.3.2 dev system I was no longer able to reproduce the original issue and the pager works as well. I think immediately getting a client error instead of an OOM is the expected behavior for now and fixes the DDoS potential, which I guess is why this was reported as a private issue previously. So we can cross those off the list. I also tested the patch on two systems using Search API Solr, which use pagers from contrib/custom. Everything still worked as far as I could see.
Looking at the difference of the original approach vs. the new approach, ViewsExecutable initializes the pager, if needed, before returning it, which means assigning the pager object from the display handler to the views executable and then also resetting the items to display and the offset on the pager object. So the difference boils down to resetting the items to display and the offset. Checking all pager plugins that come with core, this shouldn't make any difference for us since the stuff we're checking doesn't depend on any of these values.
I can't rule out that this might make a difference for implementations in contrib or custom, but I think the problem with the old approach was that we were initializing the pager too early in some cases, because our handler seems to be called multiple times per request anyway and with the invalid pager parameter then ends up in an endless loop that finally triggers the OOM. So if this works correctly, even if something in the pager plugin would depend on those additional values set by ViewsExecutable, our handler should be called again after the pager has been properly initialized, so even then the end result should be correct.
I think the next step to get this resolved would be adding the patch to the MR so that we can have a look at the pipeline results. Hopefully it should be green. I don't think we have good test coverage of the pager stuff, otherwise we should have caught the regression before it went in, so once the green pipeline confirms that nothing broke with this fix, testing the new approach with an exposed pager manually would probably also be a good idea, I didn't do that yet.
@jverneaut Would you be able to add your patch to MR 97, so that we can check the pipeline? That would be great. If needed, we have good documentation → of the MR workflow. Setting back to Needs work for that. Thanks.
Tested the MR on a dev system previously affected by the pager bug and the pagers still work. Unfortunately, the MR still does not fix the original issue for me on 10.3.2. Put the core patch on my list for review, if no one else beats me to it.
Ah yes, I think this is what core also does to initialize the pager, if it is not yet initialized? I'll give this a test.
Update: Patch #19 still fixes the pager issues I had, no further problems reported so far where we deployed this as a hot fix. Bad news: I was able to consistently reproduce the original problem mentioned in the IS with #19 applied (tested on 10.3.2). Are you sure #19 works as a fix for the original issue?
Didn't get to the bottom of this yet, but I ran into this today and deployed #19 as a hotfix. For now it works for me (i.e. the pagers of views configured to use bef work again), but I didn't try to reproduce the original issue and didn't do any thorough testing yet. I'll keep you advised.
@feyp I don't think there is anything to do with drush here. This is a core issue, and drush is not used to execute these commands.
Yes, I know that, of course. I was referring to the implementation of drush sql commands, which also call mysqldump, as a reference implementation on how to suppress this warning in code and was suggesting to do something similar here so that you don't run into this warning when running the tests locally.
> [Warning] Using a password on the command line interface can be insecure.
Last time I looked, drush created a temporary file using tmpfile() with the login data and passed that to mysqdump / mysql via --defaults-extra-file. Given that the temporary file is deleted by PHP at the end of the execution of the process and we're only using this during testing and not in production, this might be sufficiently secure, at least more secure than passing this on the command line or using environment variables (on the other hand, I guess we're already using env vars during testing anyway, so nothing gained over using env vars?). We could also re-check drush sql commands before we start implementing this, maybe there is now another solution that is even better?
Let's remove even more references to modern Drupal versions so that we don't have to keep them up to date.
Looking into this a bit, the method is called from two places. Once in the submit handler of ViewsExposedForm and then in ViewExecutable::_build().
The first call triggers the warning for this filter configuration, because at that time, the exposed input is not yet converted from the value of the exposed form element to the structure actually expected by the handler. You get an array with the selected option groups, e.g. something like [1 => 1, 2 => 2, 3 => 3], if you have three option groups and all are selected.
The parent method in FilterPluginBase will handle this and accept the input, but then the check in the switch statement will trigger the warning, because a) there is no handling for multiple values and b) the values have not yet been converted to the expected structure [min => .., max => .., value => ..].
For the subsequent calls from ViewsExecutable, before calling the handler the input will be properly converted to the expected format and then checked for each selected option group separately, so the NumericFilter will then accept the input via the switch statement.
This might have been designed this way with the idea that the exposed input for grouped filters is initially checked by the parent method only and then the real check only happens in ViewsExecutable. If this assumption was correct, then checking whether the array keys are set would be the correct fix at least for this filter configuration.
If we don't want to go with that, I think we would need to change the submit handler of the exposed form so that it checks the value in a similar way to how it is done in ViewsExecutable::_build() for this kind of filter configuration, i.e. convert everything to the expected format and then check each value separately.
@j. ayen green Thanks for letting us know. This is kind of an old issue that has already been fixed. I can see the permissions added in this issue just fine on Drupal 10.2.7 with the latest stable 4.0.1 release. If you don't see them, I'd consider this a bug. In that case, I'd ask you to please open a new issue, so that we can look into the problem. Please make sure to include your core version in the issue summary, in case there was a recent regression. Thanks.
Had a similar issue with a cacheable response from a rest resource. I didn't try downgrading, but instead moved the dynamic page cache into redis for now.
Speculating that 📌 Memcache performance issue from big (i.e. chunked) cache items Fixed might have introduced a regression?
@nod_ just committed the parent issue so this is now ready for someone to work on. Setting to Active.
As mentioned in a previous comment, the patch no longer applies to the latest release. Also, and this is already mentioned indirectly in the issue summary, the requirement to return a string for menu link titles is a limitation of the Drupal API. It might or might not be possible to return a render array for now without core patches (I didn't look into this in detail, yet), but if it works right now it is not at all certain that this would continue to work in the future.
We might actually be able to include a fix for this where we try to render any render array for the link title into a string in isolation and then return the rendered string as the title of the menu link. It is not certain that this would work, but it is worth a try. Anyway, unless someone provides an MR with test coverage, I would need an example of the way the menu link is altered to show the icons, so that I can look into this further. This could either be steps to reproduce with a contributed module, contributed theme, or more information on how this has been implemented in custom code.
Released as part of 1.0.0-beta6.
Merged.
Manually tested on 10.2.7, 10.3.1 and 11.0.0-rc1. RTBC.
Rebased the MR and added an additional commit to declare compatibility for the test module. We pass the whole pipeline now. Still needs manual testing.
Merged.
Test-only pipeline fails as expected. Passes as expected with fix. Since the fix has already been reviewed in core and the test results look as expected, I'm going to RTBC this.
Prepared an MR.
Adjust issue title
Merged.
Adjusting issue title
Just looked at the code again and with the new tests based on comment #4 I'm confident that this works as intended. So I'm going to RTBC and then merge this. I'll let you know when I create a new release.
@lelkneralfaro Feel free to open a new issue, if you still have problems after this has been merged and released. Thanks again.