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.
Alternatively, you could use a more general approach as well, similar to what is in the example template file:
foreach ($variables['items'] as $id => $item) {
if (str_starts_with($id, 'language_switcher_menu.language_switcher_link')) {
$langcode = $item['original_link']->getOptions()['language']->getId();
$title = match ($langcode) {
'en-gb' => 'en',
default => strtolower($langcode),
};
$variables['items'][$id]['title'] = $title;
}
}
With this module enabled, this preprocess function could be a lot simpler. The following worked for me:
if (isset($variables['items']['language_switcher_menu.language_switcher_link:en-gb'])) {
$variables['items']['language_switcher_menu.language_switcher_link:en-gb']['title'] = 'en';
}
Any module that tampers with the ModuleInstaller or Core info parsing for
example may wish to run those types of tests to validate that the code is
functional and only operates when intended.
That's kind of funny, because I've got such a module and I'm testing all of that. And I have a lot of test modules in that module, but not for different version constraints, there are actually other ways to validate that part. But I get what you had in mind now, I agree there are probably a small number of modules outside of core, that might actually need that indeed. I'm not sure that means we shouldn't do it by default, though. As you say, people with "exotic" needs could always modify the pipeline. Maybe we could add a flag to opt out?
If the best practice was to not use version constraints for test modules anyway, then it would be an entirely different discussion, but as I said earlier, I'm honestly not sure what the best practice is in this case.
Should we be updating the info.yml for TESTING modules?
phpunit (next major) just failed for one of my projects, because one of the test modules can't be installed on 11 due to the core requirement in the info.yml. That means that the job is pretty useless for that project right now.
What kind of scenarios are you thinking of where something like this shouldn't be updated by the job? If we are updating the main info.yml and the info.yml of sub-modules, wouldn't it then be the most logical choice to also update the info.yml of test modules?
I see some merit in not modifying anything, because then it is also less likely that you forget to make these changes when you are working on compatibility with next major, especially since the upgrade status job doesn't seem to show required info.yml updates at least for test modules (not sure about regular sub-modules right now). But I also think it doesn't make sense at all that I don't have to update the requirement for the main module or for sub-modules when I want to use phpunit (next major), but I need to do this for test modules. Also, project update bot added support for updating info.yml updates for sub-modules and test modules/themes a few months ago, so it is less likely that you miss this now when merging these MRs.
Alternatively, if we end up not doing this, then I would at least document how to use a before script to do this yourself, if desired. An other alternative would be to document that we recommend to remove core version requirement from info.yml files for test modules. If that is really the recommendation.
Thanks for filing this issue. As noted previously in #5, this doesn't reproduce by just following the steps you provided. There must be something else involved. Can you share an example of the array you're getting there? Are you using a contributed module or theme that might modify the switch links? Which one? If it is custom, would you be able to share the implementation?
Sorry, it took me a while to get back to this issue.
I'm not really sure how the patch posted in #4 would help here and I think it also doesn't apply to the latest version of the module.
Unfortunately, I can't say why your preprocessing function doesn't work without seeing the code. But hook_language_switch_links_alter() is a module hook. It is not invoked for themes. So if you want to use this hook, you have to do it in a custom module, not in a theme.
Alternatively, there are a lot of contributed modules out there that can be used to modify the links in several ways, which can be used in combination with this module.
For example, https://www.drupal.org/project/language_switcher_extended → has an option called "Show language code", which does exactly what you want to do.
There is also https://www.drupal.org/project/language_switcher_langcode → , but that module uses all uppercase instead of lower case.
There are probably more modules you could use in combination with this one that provide this feature, I stopped looking after I found those two.
I just pushed the fix for the issue, which is pretty simple. If there is no link for a LanguageSwitcherLink, we now return <nolink> as a route name instead of an empty string. With this one line fix, we pass the new test.
@lelkneralfaro It would be great, if you could confirm, if the MR resolves your problem. Thanks.
Pushed a test for the problem reported here, the tests pass in 10.2 and fail on 10.3, 10.4 and 11, as can be seen here: https://git.drupalcode.org/project/language_switcher_menu/-/pipelines/22...
Crediting @dshields here for filing an issue and providing a wip patch over in 🐛 Drupal 10 compatibility Closed: duplicate about one of the deprecated code pathes we call that I just fixed in the MR as part of this issue.
Thanks for reporting the issue and for providing a patch. It is not actually needed for compatibility with Drupal 10, but it is required for Drupal 11 as it fixes a deprecated code path to be removed then. As you can see from the test results, the patch was not quite ready as it failed tests. I just pushed a fix for this over in 🐛 [10.3] Menu containing language switcher link non-editable Needs work along with a fix for another deprecated code call as I was fixing the tests for 10.3 there and phpunit complained about the deprecated code. That issue will also bump the minimum required core version to 10.1. I'm going to close this as a duplicate and credit you over there.
Okay, I spoke too soon. Looks like the test failure is only due to the intentional behavior change in core in 10.3 that was introduced in
🐛
[regression] Do not bypass route access with 'link to any page' permissions for menu links
Fixed
. Previously, links linking to <current> were visible to users with permission to link to any page, now they're not. Since we're modifying access checks for this route, we're testing that the core behavior for any links not generated by our module is unchanged, so this failed. Once I modified these tests to reflect the intended change in behavior, the rest of the tests pass on 10.3, which means that the functionality of the module per se is unaffected by this change. So this is not actually as critical as I thought when seeing the test results. Back to normal.
I'm pushing a fix for the tests on 10.3 including two fixes for deprecated code that was also caught by the test run. This will bump the minimum core version to 10.1.
Still going to look into this.
Working on a fix.
Pipeline looks as expected. We're going to fix Drupal 10.3 compatibility in 🐛 [10.3] Menu containing language switcher link non-editable Needs work and add Drupal 11 compatibility in 📌 Automated Drupal 11 compatibility fixes for language_switcher_menu Needs review .
Thanks. I just setup GitLab CI and just looking at the pipeline I can see that tests passed on 10.2, but are broken on 10.3. Bumping to critical.
Thanks @pooja_sharma, the final state is exactly what I asked for.
Alright, I verified yesterday that the permissions now in use are the minimum required permissions. I also verified that previous review comments had been addressed. Then I looked specifically at the places where we assert that something is not on the page to make sure that this doesn't pass now just because we are missing some permissions now. Luckily those very only few places, because we mostly only assert what's there. So the code review looks good now.
There are no super user flags left to be removed within tests in the scope of this issue. The tests pass with the changes. Rest of the pipeline also looks good.
The issue is scoped appropriately. I'm gonna move the files from the remaining tasks section to the proposed resolution and then add the MR to commit, because we have multiple. Then the IS is fine. The issue title could be improved, but is okay as a git commit message and it makes sense to keep this similar to the other issues already committed as part of this meta. Follow up for performance optimization is filed.
Thanks again @pooja_sharma for staying on top of this and for understanding my concerns.
I think the issue is RTBC! „🎉
There is follow-up already created for it
No, the follow-up is to merge the to test methods, which is definitely out of scope for this issue.
But my understanding is, also from looking at the reviews by others in this issue and other similar issues, who are a part of the same meta, that the intention is to use minimal required permissions only. And the other test in FieldUIRouteTest doesn't need any permissions at all to pass. This is also underlined by an earlier review comment from @smustgrave "The move to setup() is probably fine since this is the only test in this file." In this case we have two tests, so it is different. So I think we should move the login method into the test where it is needed as part of this issue so that the other test runs without any elevated permissions. But if you want, I can ask for a second opinion. Or wait for others to RTBC this.
Thanks @pooja_sharma. As far as I can see, the login in FieldUIRouteTest is still done in the setup method. Can we please move this to testFieldUIRoutes(), because I don't think it is needed for the other test in that file? Or is there a specific reason why you think it shouldn't be done?
After conferring with @smustgrave, I filed #3460654 for a small performance optimization as a follow-up issue. For now postponed on this one so that we can get this done first.
Thanks for working on this issue. I think there are even more permissions that can be removed. Left some comments inline.
@pooja_sharma Alright. Then I missed something in the upstream changes that caused the conflict. Sorry for that. But why did you now update the remaining tasks again? You completed the update for this file, so this is not a remaining task, it is done now. Can you please change back the IS?
@kim.pepper Thanks for taking the extra review off my list, I appreciate it :)
@pooja_sharma Unless I'm missing something here, I think the last rebase was not really required. So for next time: When you rebase an MR, the issue needs to go back to Needs Review. A second review is usually quicker than the first one, because the reviewer is already familiar with the issue and your changes, but it still takes time for you to do the rebase and for the reviewer to (re-)review. So once an issue is RTBC (or even Needs Review, you don't want to rebase while someone is in the middle of reviewing your issue and then they have to start all over again from scratch) you want to rebase only, if it is really necessary. As long as there are no merge conflicts, GitLab will do a rebase automatically, once a core committer merges the MR. A merge conflict usually happens, if there are more recent changes in the main branch that modify one or more of the same files that you are modifying in the MR. GitLab will usually also show you, if there is a merge conflict and a rebase is required. You can see this by looking at the overview page of the MR. So if you are unsure whether a rebase is really needed, it is usually enough to open the overview page of the MR in GitLab and it will tell you: Instead of "Ready to merge by members who can write to the target branch." there will be a message "Merge blocked: 1 check failed" and "Merge conflicts must be resolved.". Note that you must have access to the issue fork for this to work, so if you don't have access yet, you need to request it via the issue.
I found a lot of other examples in file module where we are using uid => 1 Are we sure we can't remove these special behaviours either?
Without using the super user policy, these tests don't use any special behavior so the UID does not need to be changed. It is then just a regular user without any special access. That's also why those tests didn't have the flag added in the first place and why I said in my review in #13 that it would have been enough to change the user in just one place basically to complete this issue.
So yes, we could change the UID for the sake of not using UID 1, but it is not required.
Setting back to Needs review, because the MR was rebased.
Forgot to add the tag, sorry for the noise.
Thanks for working on this issue!
Okay, so this was interesting. The test fails, because we validate the file in two cases and the default user created in the base test with the uid 1 is not active (status != 1), so it can't be referenced, which triggers an extra violation that we're not expecting. The UserCreationTrait on the other hand always creates the user with status = 1, so it is not affected by this.
Of course it is probably not a good idea to just add status = 1 to the base test. So I think creating a new active user as it is implemented in the MR is the correct fix. Technically, it would have been enough to just use this new user id for the two files where we call validate() later and leave the rest as is, but I think it doesn't hurt here to use the active users to create the other files as well, so I won't block on this minor nit.
The IS looks good. Updating remaining tasks and adding MR to commit as we have multiple. The issue scope seems appropriate. The issue title could be improved, but it seems acceptable as a git message and I guess it makes sense to keep that in line with the other similar issues that have already been committed, so going along with that.
Code review looks good. Tests pass with the changes. No further tests requiring super user in file module left.
The issue is RTBC 🎉.
Rebased the bot's MR to include the new pipeline. Added a commit to remove deprecation helper as mentioned in #4. With those changes, the pipeline passed 100% on D10.3, D10.4.x and D11. In the future, we will maintain compatibility with D10 and D11 by monitoring the pipeline, especially the upgrade status job, that should provide us with similar results to the bot. So we can close this issue now.
Going to release this shortly.
Thanks bot and D11 readiness team!
Just noticed something else: The cspell job doc says:
For
.mdfiles you can use the comment style[//]: # cspell:disableand[//]: # cspell:enable
For the project I'm switching to GitLab CI today, I decided to include README.md this time, so I had to ignore a word. And it doesn't work with the above-mentioned syntax as you can see here the comment is shown prominently at the top of the file in GitLab.
According to some blog post, you'd need to add parenthesis around the commented text. I didn't try that but opted to use the HTML comment style instead, because when reading the post I remembered that I had used that previously and it had worked. Indeed that hid the line in the GitLab output and the cspell job still succeeded:
And one more thing I was wondering about when reading the cspell documentation was: If I include my own dictionary, will the job use my dictionary on top of the Drupal Core's dictionary (that's what I would prefer) or will my dictionary replace Core's dictionary? I didn't yet try that or check the code in the template, but maybe, if you happen to know how that works already, it would be nice to add that as well.
Thanks for filing this issue. I can't reproduce with current Drush 12.5.2.0, Drupal 10.3.0, PHP 8.3.8. Looks like this might have actually been fixed by Drush in the meantime. Can you still reproduce the issue?
Committed. Thanks for your patience.
Looks good now, thanks.
Looks like the page for the upgrade_status job needs to be added to the sidebar. As far as I can see that is the only job missing. I didn't check the other sections of the nav.
Thanks for filing the issue. I didn't yet have the chance to look at it, but just a few questions from reading the IS and your comment:
1. Did you use that same alter hook and the module on 10.2 without problems?
2. Could you maybe share the code of the alter hook? I'm assuming it is fairly simple, but it might be easier to reproduce, if I can just reuse your implementation instead of writing my own.
3. Could you share the exact error message (and possibly a stack trace, but that is not really necessary) that you get when you try to edit the menu?