Account created on 22 October 2010, over 13 years ago
  • Senior Developer at werk21Β 
#

Merge Requests

More

Recent comments

πŸ‡©πŸ‡ͺGermany FeyP

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;
      }
    }
πŸ‡©πŸ‡ͺGermany FeyP

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';
    }
πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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?

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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 .

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

FeyP β†’ created an issue.

πŸ‡©πŸ‡ͺGermany FeyP

FeyP β†’ changed the visibility of the branch 3439835-fix-field-ui to hidden.

πŸ‡©πŸ‡ͺGermany FeyP

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! β€žπŸŽ‰

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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?

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for working on this issue. I think there are even more permissions that can be removed. Left some comments inline.

πŸ‡©πŸ‡ͺGermany FeyP

@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?

πŸ‡©πŸ‡ͺGermany FeyP

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

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

Forgot to add the tag, sorry for the noise.

πŸ‡©πŸ‡ͺGermany FeyP

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 πŸŽ‰.

πŸ‡©πŸ‡ͺGermany FeyP

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!

πŸ‡©πŸ‡ͺGermany FeyP

Just noticed something else: The cspell job doc says:

For .md files you can use the comment style [//]: # cspell:disable and [//]: # 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:

  1. Relevant commit
  2. Rendered Markdown in GitLab
  3. Successful job in the upstream project

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.

πŸ‡©πŸ‡ͺGermany FeyP

Merged.

πŸ‡©πŸ‡ͺGermany FeyP

FeyP β†’ created an issue.

πŸ‡©πŸ‡ͺGermany FeyP

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?

πŸ‡©πŸ‡ͺGermany FeyP

Committed. Thanks for your patience.

πŸ‡©πŸ‡ͺGermany FeyP

Looks good now, thanks.

πŸ‡©πŸ‡ͺGermany FeyP

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.

πŸ‡©πŸ‡ͺGermany FeyP

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?

πŸ‡©πŸ‡ͺGermany FeyP

Thanks @acbramley! I created an MR against the 2.x branch, that should be all that was missing to meet the contribution guidelines for NR.

One small note on this part of the code in TokenProvider:

      $default_layout = $this->diffLayoutManager->getDefaultLayout();
      if ($default_layout === '') {
        $replacements[$original] = $this->t('(No default diff layout configured.)');
        continue;
      }

This was an empty() check in the original patch for 1.x, because I actually ran into this during development. empty() checks are no longer allowed by the phpstan configuration of this module. Thus I looked at this again and the method has now a string return type hint. So I changed this to check for an empty string instead. But: getDefaultLayout() still has this return statement return \reset($plugins); and if $plugins is an empty array, it will not return a string, but boolean false. This means that we could probably remove this check in TokenProvider entirely, since the Layout Manager will then generate a white page (wrong return type). Or we could fix the wrong return type. For now I didn't change anything about this in the layout manager, because unlike the other change in the layout manager that is a part of this patch, this bug fix is not needed to pass tests here, so it could be done in a follow-up issue, if desired.

πŸ‡©πŸ‡ͺGermany FeyP

Created the MR for both 1 and 2 from the proposed resolution. Suggested Composer commands tested on Drupal 10.3 and Composer 2.7.7. Tested that merging into existing scaffolding plugin config works as expected. Could use verification of my test results and maybe some proof-reading from a native speaker. Alternatively, the directory name is easily changed with the old text to just do part 1.

πŸ‡©πŸ‡ͺGermany FeyP

Various steps in here refer to a remote named drupal-ISSUE_NUMBER. In some cases, these are probably examples for the Core example mentioned in the introduction, in which case this would be correct, but not always. The drupal part is actually the machine name of the project you're working on. So I think this should be updated in those places that do not directly refer to the example in the intro. For now, I didn't do it, because I'm not really sure if this documentation should only apply to core development. I ended up here from clicking the "about issue forks" link in a project issue and then clicking a link in the sidebar, so I think it should apply to all projects, not just core?

πŸ‡©πŸ‡ͺGermany FeyP

@smustgrave Sure. I pushed a new branch for 7.0.x to the fork and updated the code there to address my review. New MR for 7.0.x is MR 82. I also rebased MR 6.0.x and cherry-picked my commits there, pushed everything to MR 72, which is still for 6.0.x.

I tested the update hook on Drupal 10.3.0 with the 7.0.x MR and I think it works as intended now. Note that I didn't re-test the 6.0.x MR after my changes, although I don't see how this shouldn't work.

I added the string change for the description as a separate commit so you can easily revert it, if you don't agree with that one. Unfortunately, it looks like I don't have permission to resolve my own review comments on the MR 72, so they're still open.

Here is a config export after the update hook with a single BEF view, only change was this one, just as intended:

--- a/sync/views.view.better_exposed_filters_issue_3282228.yml
+++ b/sync/views.view.better_exposed_filters_issue_3282228.yml
@@ -124,6 +124,7 @@ display:
                   collapsible: false
                   collapsible_disable_automatic_open: false
                   is_secondary: false
+                  hide_label: false
                 select_all_none: false
                 select_all_none_nested: false
                 display_inline: true
πŸ‡©πŸ‡ͺGermany FeyP

FeyP β†’ changed the visibility of the branch 3282228-add-option-to to active.

πŸ‡©πŸ‡ͺGermany FeyP

FeyP β†’ changed the visibility of the branch 3282228-add-option-to to hidden.

πŸ‡©πŸ‡ͺGermany FeyP

Did a little bit of testing using the workaround patch in #4 and for now it looks like it works as promised (i.e. the bug is fixed with the mentioned limitations).

Let's set the issue status to needs work, since there is now a patch and there is also a more concrete proposed resolution in #4.

πŸ‡©πŸ‡ͺGermany FeyP

Also proactively tagging for IS update, since we're missing a proposed resolution. Would be good to update the IS to the latest state before setting to needs review again as well, otherwise the issue will surely fail the review for that.

πŸ‡©πŸ‡ͺGermany FeyP

Can anyone explain why this has been set to needs work?

It is needs work for comment #72.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for working on this issue. I have a use case for this functionality in one of my projects, so I did a quick review. Unfortunately, I noticed an issue with the update hook. I confirmed that the update hook probably doesn't work as intended on a patched BEF 6.0.5 on Drupal 10.2.6. Since we're touching the code again anyway, I also added some rather nitpicky stuff as well, which is totally optional.

πŸ‡©πŸ‡ͺGermany FeyP

Add deprecation of class properties.

πŸ‡©πŸ‡ͺGermany FeyP

It seems that the second link is what you would expect:

Yes, that was the page I expected. Thanks for pointing that out.

Still, on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi... I would expect, that FileSystemInterface in the class hierarchy links to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi... , which it doesn't. Those are different pages.

And I'd expect the box "Same name in this branch" on https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa... , so that I can at least quickly switch to the page I'm looking for.

I think that this "services" detail pages might not be needed at all.

The services detail page is fine, if it links to the service class, but I agree it is probably not needed under the name of the interface the service class implements.

So this is useful: https://api.drupal.org/api/drupal/core%21core.services.yml/service/file_...
I think we can drop this one: https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa...

πŸ‡©πŸ‡ͺGermany FeyP

When searching for FileSystemInterface in 11.x branch, the result doesn't look right. I get a page for the @file_system service in core.services.yml. The class providing this service implements the interface, but I'd expect a page listing all the constants and methods in the interface. I also don't see a "same name elsewhere in this branch" box (or whatever is the exact name of that box), so it can't be just the "wrong" page:

https://api.drupal.org/api/drupal/core%21core.services.yml/service/Drupa...

On the page for the FileSystem service, it says it implements FileSystemInterface, but there is no link, so it seems it is indeed missing:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi...

And thanks for fixing #32 and for the interesting link to your presentation @fjgarlin .

πŸ‡©πŸ‡ͺGermany FeyP

Cool, you're using search api. If you ever write a case study on the API page, make sure to share the link with us, it might be interesting. And no rush on the reindex from my side, as long as the content is what I expect, I'm happy :)

πŸ‡©πŸ‡ͺGermany FeyP

Is it expected that I see `https://api.prod.cluster.drupalsystems.org/` in the URL on production instead of `https://api.drupal.org`?

Steps to reproduce:
- Go to api.drupal.org
- Search for `NodeInterface` in the sidebar.
- Click on the result/press enter.

Expected result:
- Redirect to `https://api.prod.cluster.drupalsystems.org/api/drupal/core%21modules%21n...`

Actual result:
- Redirect to `https://api.drupal.org/api/drupal/core%21modules%21node%21src%21NodeInte...`

Still doing more testing, but for now it looks great :)

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for filing this issue.

We don't actually provide the titles of the links in the module. The module uses the links (including the link title) from Drupal Core's language switcher and makes them available for display as menu links.

If you want to change the language switch links, including the link titles, there are a bunch of contributed modules available that you should be able to install in addition to this module that allow for various alterations of the language switch links. Alternatively, you can use hook_language_switch_links_alter() in a custom module.

For your specific request, there is also an example in the FAQ on the project page that I think does exactly what you asked for by just changing a template file in your theme.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for working on this issue. This looks very similar to πŸ› Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() Needs work . The other issue doesn't have an MR already, but is older, has a patch with a kernel test, has way more followers and even more recent activity, so I'm going to close this one as a duplicate. It would be great, if you could continue working on this in the other issue. Feel free to reopen, if I missed something. Thanks again.

πŸ‡©πŸ‡ͺGermany FeyP
-      watchdog_exception('patchinfo', $e);
+      \Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('patchinfo'), $e), fn() => watchdog_exception('patchinfo', $e));

Noting that DeprecationHelper was introduced in 10.1, so using it to replace watchdog_exception() doesn't make sense in this case, because Error::logException() was also introduced in 10.1. Either way we'll have to bump the version constraint to 10.1 for the next version. So in this case we can just use the new API without using DeprecationHelper.

This is a known limitation of the update bot/rector, because it made the implementation less complicated. Likewise, it is a known limitation that info.yml files of sub-modules are not updated by the bot for now. The latter issue might change at some point in the future, but most likely not during this cycle.

We could already fix the watchdog_exception, but let's keep the version constraints as is for now until at least beta1 or even rc1, in case there will be new deprecations we need to deal with later.

πŸ‡©πŸ‡ͺGermany FeyP

fwiw +1 from me as well. The wording sounds good to me, but I'm not a native speaker so ymmv. A few observations:

Since this is for posting to issues, I'd use proper anchor tags for the links with a nice title. I think it looks better and also might give more information on what the material in question is about, e.g. "Video: Introduction to Contribution Best Practices for Organizations" instead of a random link to Youtube.

For my personal usage, I'd word "I will not be be providing credit for this issue." a little bit less explicitly, similar to "This issue may have been reported" further below. As a maintainer I might still choose to give credit to some of the people involved in the issue, but might still want to inform them using this nudge that it was an exception to the rule and please be more careful next time. I might apply this exception for obviously novice contributors or for people that provided constructive input on the issue after it had been already created for some time without any movement/reaction to my review from the original authors/contributors. My plan is to also use this for issues that have been created before the policy came into effect, so I'd like to post this nudge as a hint to it so that they are aware of it for the future, but might still give credit as an exception this time. I can surely adjust the wording as needed, but if such situations would be covered by default by slightly rewording that sentence, that would be ideal.

πŸ‡©πŸ‡ͺGermany FeyP

warning: "Warning: Give to trusted roles only; this permission has security
implications. It may lead to sensitive configuration values that are
specified in settings.php to become visible in the user interface, such
as API keys, tokens and other secrets."

Wording the message in this way may lead a user to the wrong conclusion, that leakage of such info is the only security implication, which is often not the case. Depending on the module, this might actually not even be the most severe implication, but rather one of the minor implications.

Also fwiw +1 for #238.

πŸ‡©πŸ‡ͺGermany FeyP

I should have updated the proposed resolution in the IS also, done that now.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks @alexpott for the review.

W/r/t pointing out LanguageInterface::isLocked(), I closed the follow-up ✨ Add an API method to LanguageInterface to simply checking, if a language is a pseudo language Closed: works as designed issue as "Works as designed".

I agree that the changes you suggested are shorter, more logical and more readable, so I updated the code.

I took the opportunity to switch to an MR based workflow. Added the MR under review to the IS as requested elsewhere by @xjm.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for working on this. @alexpott pointed out over in #2904899-29: Invalid argument exception when changing language of node with menu link to und or zxx β†’ that you can actually use $node->language()->isLocked(), if you need this, so I'm going to go ahead and close this as works as designed. Feel free to reopen, if you disagree.

πŸ‡©πŸ‡ͺGermany FeyP

I wasn't able to reproduce the test fail locally, so I used the opportunity to switch to an MR based on patch #44. Let's see what the pipeline says.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for reporting this issue. I tried to reproduce on Drupal 10.0.11 and unfortunately, I wasn't able to do so. I did the following:

  1. I created a vocabulary named "categories".
  2. I created an entity reference field "field_test" for the user entity, referencing taxonomy terms and restricted it to a vocabulary named "categories".
  3. I created two published terms in the "Categories" taxonomy, "Term 1" and "Term 2".
  4. I set up a role and gave it none of the permissions provided by Taxonomy or by Taxonomy Access Fix.
  5. I created a user with that role and set field_test to reference "Term 1" for that user.
  6. I added a hook (code below) to hide field_test for the edit operation and cleared all caches for the hook to become active.
  7. I logged in as the new user and verified that I wasn't able to see field_test on the user edit form.
  8. I saved the form.
  9. I edited the hook to apply to field_xtest instead of field_test.
  10. I looked at the form again both as the user I created and with user 1. For both users, the field was visible again on the form. With user 1 I was able to see the reference to "Term 1". For the user I created for testing, the field showed "- Restricted access - (1)".
  11. I tried this again editing various other fields on the user edit form instead of clicking just save when the hook was active, no luck.

Here is the hook I used for testing:

use \Drupal\Core\Access\AccessResult;

function example_access_fix_entity_field_access($operation, \Drupal\Core\Field\FieldDefinitionInterface $field_definition, \Drupal\Core\Session\AccountInterface $account, \Drupal\Core\Field\FieldItemListInterface $items = NULL) {
  if ($field_definition
    ->getName() == 'field_test' && $operation == 'edit') {
    return AccessResult::forbidden();
  }
  return AccessResult::neutral();
}

I'd appreciate it, if you could provide more detailed info on how to reproduce the issue. Do you use a different version of core? I didn't yet have a chance to test on 9.5 or 10.1. Can you share your hook, maybe there is something different that triggers the issue? Anything else I might be missing? Any other contributed modules/custom code you use that might be candidates needed to trigger this?

πŸ‡©πŸ‡ͺGermany FeyP

Add new "Theme templates" section to the TOC.

πŸ‡©πŸ‡ͺGermany FeyP

Reading this again, router => router was a bad example. Let's use an example where it's more obvious what the property name is and what is the service name. Also, the service previously assigned to the property must not necessarily be deprecated.

πŸ‡©πŸ‡ͺGermany FeyP

Add a section on how to deprecate injected service properties using \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait, discussed with @lauriii in Slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1698411358577049

πŸ‡©πŸ‡ͺGermany FeyP

Attached is a patch against 2.x-dev.

πŸ‡©πŸ‡ͺGermany FeyP

@kunal.sachdev Thanks for creating the follow-ups. Can you please set the status of the follow-ups to Postponed and post a comment that they are postponed on this issue when you change the status? You can also add this issue as a parent issue or as a related issue. I don't think it makes sense for someone to work on these issues until this issue is fixed.

Also, I think it might be a good idea to list the follow-up issues somewhere in the issue summary, because this issue is pretty long and takes some time to read through, so a good issue summary that has all the relevant information is even more important for this issue to make it easy for core committers and other contributors new to the issue.

The issue summary also lists a remaining task (2.) that is not marked done yet. Can you please update the IS with the current approach in your implementation and/or with a reference to the follow-up you created, so that it is clear that the task is either done or out of scope?

W/r/t the last follow-up, a subsystem maintainer mentioned in #48 that the setting should be kept in field storage. Unless there is compelling evidence that the reasons given by them are maybe outdated in the meantime (they commented 5 years ago, so it might no longer be true due to some other changes in core), I think we should follow their advice and keep the setting in storage. If we think the setting should be in the field settings, I don't think we should have a follow-up, but I think we should do it in this issue. It doesn't make much sense to introduce a new setting now and then shift that from storage to field settings later. We could then use a field setting right from the start.

πŸ‡©πŸ‡ͺGermany FeyP

I could fix this only by deleting the cache tables in db. drush cr, did not solve it.

That's interesting. So it's not related to the code at all, but some kind of caching issue? I'll see if I can find something in that direction. Might be a few days until I can take a look though.

πŸ‡©πŸ‡ͺGermany FeyP

I think #87 is covered by πŸ› Claro: Form labels that are disabled have too low color contrast Needs work , which mentions border color of disabled elements in the IS, so we don't need a follow-up for that.

Form API states has various issues already filed against it, but doing a quick search, I didn't find any obvious candidates for #92. The tests only cover usage with TRUE, not with FALSE. I think we should file an issue for this, if it doesn't exist already, but I don't think we need to postpone on this. But I also think we shouldn't commit it like this, so let's go back to the previous behavior. We could then file another follow-up to use the current behavior using Form API states in this case to improve usability and post-pone that PP2 on this issue and the new follow-up for Form API states.

Tagging Needs Followup again for that.

πŸ‡©πŸ‡ͺGermany FeyP

I just closed πŸ› States JavaScript should use the once library, causes problems with Big Pipe Closed: duplicate as a duplicate of this issue. There is some information on how to reproduce with the contributed Webform module in the IS of that issue and MR with a similar but slightly different approach to fixing this.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for filing and working on this issue. This looks like a duplicate of πŸ› Form API #states property/states should use .once() to apply its rules (Can cause failures with BigPipe and possibly other situations) Needs work , so I'm going to go ahead and close this, since the other issue is older and has more followers. Feel free to reopen, if I missed something.

πŸ‡©πŸ‡ͺGermany FeyP

No response within a month. Closing works as designed for now. Feel free to reopen with more information.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for letting us know. Looking at the code in core I don't see any recent changes that we didn't address and I haven't had any issues myself on a bunch of systems.

Most likely this is either a core patch that you're using that modifies the route, another contributed module that alters this route or some custom code you use. Could you please check your systems for any candidates? If it's a contrib module, we might be able to accommodate with a patch.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks, good catch! I updated the existing kernel test for the processor to cover this issue. No interdiff, since it's trivial.

πŸ‡©πŸ‡ͺGermany FeyP

Attached is a patch against 4.x-dev.

πŸ‡©πŸ‡ͺGermany FeyP

Attached is a patch against 8.x-1.x-dev.

πŸ‡©πŸ‡ͺGermany FeyP

Attached is a patch against 2.0.x-dev/2.0.2.

Note that the patch adds filtering of the attribute value based on \Drupal\Core\Template\AttributeString::__toString(), only using Html::escape() directly since it was already used anyway, and a check, if the value is not empty. Technically, the first hunk of the patch should be enough to fix the bug.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for filing this issue.

The module already provides an upgrade path for those new permissions, that were previously part of other old permissions. See the update hooks 9401 and 9402.

For the other permissions, these were not previously covered by any existing permissions, they are entirely new and allow different things. We can't just give arbitrarily new permissions to roles they didn't have before. It may seem "logical" for your project, but we can't assume that it will be the case for all projects that use the module. That's why the update hooks also ask you to review the newly assigned permissions, so that you can take the opportunity of the upgrade to make changes as necessary.

I'm not closing this as won't fix yet, because I wonder why you needed to add "view terms in VOCABULARY" to "view term names in VOCABULARY" (provided by 9401) and "view terms in VOCABULARY" to "select terms in VOCABULARY" (provided by 9402). Didn't those update hooks work for you? If so, can you tell us your module and core versions before and after the upgrade and also if you upgraded in multiple steps or a single step?

πŸ‡©πŸ‡ͺGermany FeyP

Sorry, bot, but you tested the wrong patch...

πŸ‡©πŸ‡ͺGermany FeyP

Thanks @lauriii for the clarification in #43, very helpful.

I replaced the data providers with foreach loops, this should address #41. Patch and interdiff without indentation changes provided for convenience of reviewers.

πŸ‡©πŸ‡ͺGermany FeyP

@sascha_meissner Thanks for working on this. The merge is blocked because there are new commits on 10.1.x and your branch has diverged, so a fast-forward merge is not possible. To fix this, you need to rebase the MR branch. You can see this when you open the MR page in GitLab:

> Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally.

See here for more info: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... β†’

To get an early review or a second opinion for design decisions, you could ask in #ckeditor5 or #contribute in Drupal Slack.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks for letting us know about this error. I think the problem is a missing use statement for EntityTypeManagerInterface. I have just updated the example with such a use statement. That way, you don't need to change anything else. Setting the type hint to the interface is generally recommended, since it allows to inject another class as long as it implements the same interface, so I wouldn't change that.

πŸ‡©πŸ‡ͺGermany FeyP

Add a use statement for EntityTypeManagerInterface to the example in step 3.

πŸ‡©πŸ‡ͺGermany FeyP

4.0.1 has been released. New issue for the next release: 🌱 Plan for Taxonomy Access Fix 4.0.2 release Active

πŸ‡©πŸ‡ͺGermany FeyP

Now had a chance to test the various upgrade paths manually and it seems to work as intended. RTBC.

πŸ‡©πŸ‡ͺGermany FeyP

Thanks once more for working on this! All feedback in the MR has been addressed and I can't find anything new :). I did another manual test on 11.x and everything works great.

The IS looks good and reflects the current state of the issue. The CR still reflects the current state of the MR. As I said previously, since both affected forms are marked as internal, I think we don't need a separate CR for the constructor changes, so I think we're good there.

We've got test coverage for all user permission forms in core, we got test coverage for the deprecation message in the constructors, we got test coverage for the system access content logic in UserPermissionsForm including some edge cases that might happen during filtering. Tests pass locally and on CI and removing some code in a few selected places, the tests fail as intended.

I think this is RTBC! πŸŽ‰

Production build 0.69.0 2024