🇺🇸United States @dcam

dcam → changed the visibility of the branch 11.x to hidden.

I don't know what's wrong with MR 9183. GitLab reports that it can't be rebased onto 11.x, but it clearly can. It just seems to be broken. I wondered if applying the changes to another branch would work. So I exported them to a patch and opened MR 11723. GitLab doesn't have any problem with it.

Restoring RTBC status.

dcam → changed the visibility of the branch 3223022-modal-discard-dialog to hidden.

Sorry, this should have been set to Needs Review since I added the missing docblocks.

Rebased

Rebased

dcam → made their first commit to this issue’s fork.

dcam → made their first commit to this issue’s fork.

dcam → made their first commit to this issue’s fork.

Could I get a review from one of the committers, I have rebased this PR now for the seventh time.

I hear you. I rebased one for the 16th time in 3 months this morning.

Anyway, rebased again.

Rebased

dcam → made their first commit to this issue’s fork.

dcam → made their first commit to this issue’s fork.

Rebased.

Rebased. I applied the comment line length suggestion, but I'm restoring the RTBC status anyway since it's a very minor change.

dcam → made their first commit to this issue’s fork.

dcam → made their first commit to this issue’s fork.

dcam → made their first commit to this issue’s fork.

It didn't really need a rebase. Not sure why it thought the MR didn't apply.

dcam → made their first commit to this issue’s fork.

Rebased. Again. Restoring RTBC.

I added a dedicated test case. I also fixed the other test failures. The cause of those failures was a destination query parameter on the "Add content block" local action link. It resulted in a redirect loop when trying to use the action link. I expected to reply here saying "This seems like a feature request to me." Except then this business with the destination happened and it does seem like a bit of a bug. The destination is used for forms. So it's no wonder that it behaved strangely when combined with another redirect on an ordinary page.

So I think it's ok to simply remove the destination. The existing tests agree with me as they're all still passing. Or at least there's no coverage for this anomaly. It's worth noting that the node/add action link (which this issue is trying to emulate) does not have a destination.

dcam → made their first commit to this issue’s fork.

I don't know why it thinks I made an MR. It is not my intention to work on this issue.

I did look at this page earlier. This isn't the first time D.o has randomly opened MRs on my behalf after I've glanced at an issue.

dcam → made their first commit to this issue’s fork.

Form validation issues are described as having Major status.

I wrote a very basic FunctionalJavascript test that demonstrates the issue. The test fails because it finds the server-side validation error message after submitting the form. It will have to be modified appropriately after a solution for the bug has been decided, but it demonstrates that there's a problem for now. I wanted to make it verify the contents of the textarea field before submitting the form, but I don't know how. This is literally the first FunctionalJavascript test that I've written.

Anyway, I'm leaving the Needs Tests tag since it will have to be improved or modified. But I'm changing the status to Needs Review to get the attention of a maintainer for an opinion on a fix.

I verified the problem on my Windows laptop. This is still an issue with the latest 11.x-dev.

The issue summary has been updated with information about the actual problem. This isn't the result of PHP "counting new line characters twice." The problem is that in Windows new lines are literally two characters, a carriage return and line feed (\r\n). JavaScript and browsers ignore the \r when calculating string length. PHP counts them both.

Thus, the former proposed fix and patch in #3 are unacceptable solutions. They would cause strings to pass form validation, but would eventually result Drupal trying to save strings that are too long into database fields whose sizes are set to the given maxlengths. That would result in even worse errors, probably WSoDs.

I don't know what the best solution is to the problem. A framework manager will have to decide.

My plan is to move the test plugins out of the field_test module so that we don't have to depend on that anymore...

After evaluating how much work that would take I ended up putting all of the entity_test module stuff in the fixture. It's a pain, but it worked without having to refactor code to work without it.

This is going to fail tests because I excluded all the entity_test lines from the fixture that I created. But I'm out of time to work on this right now and need to save the work. My plan is to move the test plugins out of the field_test module so that we don't have to depend on that anymore, reducing the amount of garbage that has to be in the fixture.

dcam → changed the visibility of the branch 2865710-dependencies-from-only to hidden.

dcam → changed the visibility of the branch 11.x to hidden.

Not all of those views specified the content of the text, so there was nothing to update in those cases. But I updated the ones that needed a dependency.

Results of grep -rn "plugin_id: text$" --include views.view.*.yml:

core/profiles/demo_umami/config/install/views.view.frontpage.yml:303:          plugin_id: text
core/profiles/demo_umami/config/install/views.view.frontpage.yml:373:          plugin_id: text
core/modules/content_translation/tests/modules/content_translation_test_views/test_views/views.view.test_entity_translations_link.yml:104:          plugin_id: text
core/modules/views/tests/fixtures/update/views.view.test_filter_format_dependencies.yml:164:          plugin_id: text
core/modules/views/tests/fixtures/update/views.view.test_entity_id_argument_update.yml:193:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_empty.yml:34:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_empty.yml:40:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_id_argument.yml:160:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml:109:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml:141:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:50:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:56:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:148:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:154:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:161:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:167:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_token_view.yml:205:          plugin_id: text
core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.sa_contrib_2013_035.yml:178:          plugin_id: text

I applied the suggestion and rebased the MR too.

I messed up that MR while trying to update it for 11.x. I'll sort it out tomorrow.

🐛 trans filter is missing on text in status-report-counter.html.twig file Fixed was a duplicate of this one, but it got fixed and committed. The same changes were made in its MR.

I added a test. It seems a little ridiculous to create a testing profile for this issue, especially since once system's requirements hook is broken up into more manageable and testable pieces then it will be easy to test this with a Unit test. But this is a critical bug and shouldn't wait for that splitting to happen. Maybe there should be a follow-up that's postponed on 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active to convert this new Functional test to a Unit test.

The update function number was updated per the discussion in Slack. I also rebased it against 11.x to get the latest changes.

Change record added at https://www.drupal.org/node/3515212 → . Leaving status as Needs Work due to the question of what the update number should be. I checked Slack and no one has responded yet. I'll try to check back in a day or two.

I converted the patch in #2 to an MR. Then I fixed it because the return value from calculateDependencies() was incorrect. I added a test. At some point I realized that this needed an update path. So I wrote that and added a test for it too.

I did not address the question in #13. I wasn't sure if it's necessary since plugins are @internal.

Rebased again.

dcam → made their first commit to this issue’s fork.

I rebased the MR against 11.x again. Restoring RTBC status.

Yes, that's what I'm saying. But that isn't the original reason for this issue. A maintainer would need to weigh in and determine whether the issue should be refocused to do that.

While I might not care about this issue as stated (where the standard login form is used, but for some reason the password reset page must be disabled) being able to disable the password reset page with Core functionality is of interest to me. We use an external authentication provider at work. Since we bypass Drupal's basic auth, I had to build a small custom module to turn off the password reset route so no one tries to use it. There are contrib modules out there that do similar things, but none of them did exactly what I wanted. So this is a pretty common need for people using external auth.

This issue isn't particularly important to me; I'm just writing tests for issues that need tests. But for what it's worth I think it's strange idea to have a configuration setting that effectively asks admins "Do you want to prevent a link from being displayed that may lead to a 403 error page depending on your site's configuration?"

If it must be configurable, then it may be better to build the capability to turn off the password reset page directly into Core instead. That way we're checking a more well-defined feature, not building configuration whose function is difficult to articulate why an admin would want to use it.

Added screenshots.

I implemented the autoconfigure option in the services file.

@vensires Thanks for providing the patch. Since this is a bug report it will need tests in order to be accepted. We do this in order to make certain that the fix isn't accidentally removed in the future.

I have not reviewed or tested the fix yet.

I copied and pasted the $next_row variable name from elsewhere in the test class. I forgot to change it and that name isn't relevant to the new test. It bothered me, so I updated it. This is a minor change that doesn't really affect anything, so I'm leaving the issue at RTBC.

I updated the docblock, but I put the reason why I wrote it the way that I did in an MR comment.

Attempt 2 to change the project.

As mentioned in #12, there is no Link (with Attributes) widget in Drupal Core. That belongs to the Link Attributes widget module. I'm moving this issue to that queue.

I rebased the old MR for 11.x and added a test.

dcam → changed the visibility of the branch 3357616-remove-forgot-your to hidden.

dcam → changed the visibility of the branch 11.x to hidden.

dcam → made their first commit to this issue’s fork.

The given steps to reproduce the issue are not sufficient. Please provide step-by-step instructions beginning with a fresh install of Drupal Core. I used the autocomplete Authored By field both while creating and editing content without any error.

I converted the patch in #2 to an MR and added a test.

dcam → changed the visibility of the branch 3319633-language-block-role to active.

dcam → changed the visibility of the branch 3319633-language-block-role to hidden.

dcam → changed the visibility of the branch 11.x to hidden.

dcam → changed the visibility of the branch 3319633-accessibility-language-switch to hidden.

I converted the patch in #3 to an MR and added a unit test.

dcam → made their first commit to this issue’s fork.

The patch in #27 was converted to an MR. I also added a test.

dcam → changed the visibility of the branch 11.x to hidden.

dcam → changed the visibility of the branch 2845241-if-a-permissions to hidden.

Just my $0.02:
I've started to implement hook classes in my own work. A decision that I made pretty quickly was to adhere to the single-responsibility principle. This has resulted in a shift in the way that I think about hooks, which I realize was dictated largely by the fact that we could only have a single implementation per module. Now we can have multiples. We're no longer bound to lumping everything a hook needs to do into a single function.

As a result, I'm more inclined to make hook classes that focus on what function it's supposed to perform, not on what part of Drupal it alters. Granted, sometimes those are the same thing. In the case of the former, then I've been naming the class according to its purpose. For the latter, then I do name it according to the hook it contains. I like this pattern. It means that it's a little easier to know just from the class name what that class is supposed to be doing. And it means that the hook is no longer what's important - its purpose is. I've been enjoying working with them! Unit tests for hook classes are so easy to write.

I've read through this issue and another and understand that there's some performance concerns surrounding having too many hooks. And I get that Core may want to have well-defined patterns in order to prevent chaos. I just wanted to share what I've been doing as someone developing contrib and custom modules.

Would you prefer to deprecate it instead of removing it immediately?

It doesn't make sense to me that this issue was filed as a bug report. A quick search for other "dead code" issues in the Core queue shows that they're mostly/all Tasks.

I considered the suggested test:

Possibly add test for D8 to insure that the password is the hashed value after save() if no such test exists.

I even wrote one out, but in the end this is no different from a Functional test where the login form is submitted. That's covered by UserRegistrationTest. If I'm wrong, then tell me. Needing a test for this doesn't make sense to me. The other dead code issues I checked don't require a test for it either. So I'm removing the Needs Tests tag (along with other superfluous tags).

Aside from that, I also wondered why PHPStan didn't pick up on this. This line contains an undeclared class property assignment. I figured that should be setting off a standards error. But it doesn't because Core only lints with PHPStan level 1. Those warnings are activated at level 2. You can cause an error to occur manually with the command vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist -l 2 core/modules/user/src/RegisterForm.php.

The related issue to split up the File hooks was committed. The changes in the existing MR will need to be applied in the new TokenHooks class. This seems like a Novice task to me.

I just got bitten by this after upgrading a D10 site to D11 for the first time ever.

Hello new contributors at DrupalSouth! Here's some guidance for you:
You're right that this can't be reproduced with the Standard profile! The status report page specifically excludes the code that causes the bug when you're on the Standard profile. See the relevant line of code. You can reproduce this with any other profile. Try deleting the version from the Minimal profile instead!

I've updated the issue summary with new instructions.

I don't think the test changes are any good. Neither undoing the fix nor removing d7_menu from the test's list of migrations will make the test fail. So I'm leaving the Needs Tests tag in place. But I'm marking this as Needs Review in hope that a subsystem maintainer will advise on what might be wrong or an alternate test strategy.

dcam → made their first commit to this issue’s fork.

Based on #22 I removed the collapsible behavior entirely.

dcam → made their first commit to this issue’s fork.

I'm sorry, but I can't reproduce the issue with the information that is given. Here are the steps I took in an attempt to follow the steps given in the issue summary:

1. Created a new view. I chose to display taxonomy terms.
2. Added a new exposed Term ID filter.
3. Added the filter to a new filter group.
4. Saved the view.
5. Visited the view and verified that the Term ID filter works.
6. Edited the view and changed the Term ID filter's identifier.
7. Saved the view again.
8. Visited the view and verified that the Term ID filter works.

If you can tell me where I went wrong, then I'll be happy to try again.

dcam → changed the visibility of the branch 3438739-calculated-machine-name to hidden.

Added info to the steps to reproduce. This is reproducible out-of-the-box with one of core's testing modules.

I added an assertion to MachineNameTest in a new D11 branch. I haven't added the fix created by @jrockowitz yet. That new assertion failed as expected. I'll commit the fix and see what happens.

dcam → made their first commit to this issue’s fork.

I tried really hard to reproduce this issue and could not. I can see how it could happen, given some set of circumstances that likely involves a contributed module. I even installed the menu_condition module since there's a related issue in its queue about this same problem. I still couldn't make it happen.

If you are experiencing this issue, then please update the issue summary with detailed steps to reproduce the problem beginning with a fresh installation of Drupal Core.

@smustgrave I did note it with this line in the CR:

In the default core/modules/system/templates/details.html.twig template a

wrapper was added around the description with the new description.attributes.

Do I need to add more, edit the line, or note it in a different way?

I added #65 to the MR.

dcam → made their first commit to this issue’s fork.

Sorry for all the rebasing spam. I've been trying to keep this up to date despite all of the other recent commits. Practically every change to the performance test forces a rebase and update of this MR.

Removing patches

I converted #20 into an MR and added a unit test.

dcam → made their first commit to this issue’s fork.

🐛 Migrate exception message should give the index of the process plugin in the pipeline Active adds indexes to subprocess pipeline exception messages to indicate which plugin had the problem. Is this a duplicate of that issue?

🐛 Exception message in subprocess doesn't say which property in the subprocess was affected Needs work may be a duplicate. It's about exception messages produced by subprocess pipelines. But this issue adds indexes to subprocess pipelines too.

I updated the issue summary.

dcam → made their first commit to this issue’s fork.

Updated the issue summary.