🇺🇸United States @dcam

@godotislate this issue is about textarea elements. It looks like core's fields with textarea elements don't have settings for maxlength attributes. Am I missing something?

I added a test.

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

The title was changed in #46.

I implemented @quietone's feedback and added a unit test, deleting the previous changes to the functional test.

The three primary pager plugins; Full, Mini, and Some; now have identical summaryTitle() functions. This repetition is painful and may only lead to more issues in the future if only one is changed again later. Frankly, I think the changes should go in PagerPluginBase instead of having it return "Unknown." Let outlier plugins like the None plugin override this sensible default. Just my $0.02.

Also, I think it's important to note that the proposed changes remove unique text from the Full and Mini pagers. Previously, the Full pager output a string like:

Paged, @count items

And the Mini pager output a string like:

Mini pager, @count item

The "Paged" and "Mini pager" text is now gone. I don't know that it matters, but it needs to be mentioned.

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

12 and 19 still haven't been addressed...

Yeah, I wasn't sure if action should be taken to display both or not. It isn't clear. If we're displaying both, then the issue summary needs to be updated.

I added an assertion.

FWIW, as a site builder and admin my preference is to see both. If I want or need to switch to Drush, then I don't want to have to go hunting for a module's machine name.

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

Right on.

I chose:

Tests removing third-party settings when a provider module is uninstalled.

I closed the old MR because the .inc file it was modifying has been changed into a service class. There's no point in rebasing that.

I started to write a test and then decided "There is no way I'm writing a Unit test that covers an entire, complicated, nearly 500 line function." So I split the offending code out into its own function in the same class. I tried to leave it as unmolested as possible - no arbitrary code formatting changes. That way it should be easier to verify that not much changed.

dcam → changed the visibility of the branch 3204558-timestamp-field-type to hidden.

I'm daring to self-RTBC this because:

• The update was requested by a core committer who said the MR would probably be ready afterward.
• The update removed somewhat arbitrary code style changes in the MR.
• The update simplified the diff.

But since there are a lot of commits that have happened since #138 I figure that a good, old-fashioned interdiff wouldn't go amiss.

Applied all feedback. Updated the proposed resolution for info logging.

No problem.

I checked the commits that you made and they look good to me. I think that if the split test is running and passing (it is), then it's good to go.

I removed any changes not related to upgrading the version compatibility. Earlier today I attempted to apply the MR's diff as a patch so that I can use the composer lenient plugin and proceed with a D11 upgrade. Unfortunately, the diff couldn't be applied for some reason. I wondered if the unrelated changes were the problem, so I took them out. Without the extra stuff the MR could be applied, and I was able to proceed with upgrading the site to D11.

I left in my removal of D9 compatibility. You can't test against D9, so you can't guarantee compatibility. That means future changes may break compatibility and you wouldn't even know until you start getting bug reports.

I'm marking this MR as RTBC. I know that marking your own changes as RTBC is taboo in Drupal Core, but this isn't Core. It's a one-line change. If no maintainer is interested in applying the MR, then consider giving me maintainer rights and I'll merge it. I'm not crazy about the idea of taking on yet another module that past maintainers have lost interest in and I don't know much about, but I'm willing to do it.

Bummer doesn't actually solve the problem for better_exposed_filters but how does third_party_settings get added anyway?

Yeah, I noticed that, but forgot to mention it last night. Since BEF doesn't use third party settings to modify views, this fix won't apply to that problem.

Third party settings are applied to config entities via the setThirdPartySetting() function.

I added a test.

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

I responded to all MR feedback and made the suggested changes. I don't have a good explanation for the typehint removals though.

@catch Sorry I hadn't finished the rebase yet. I was busy with work.

This needed a rebase due to other commits that changed stylesheet performance metrics.

In fact, let's add the Usability tag too.

I realized that $request property isn't even used. Maybe it was from early draft of the MR or something.

Small nitpicky comment

Applied suggestion

But NW for the issue summary.

Updated.

Also not 100% I see the issue?

Like I said in my last comment, this is a feature request. Nothing is broken. I'd call it a UX improvement.

That said, I did uncover what seems like a minor bug while fixing the tests. There's an unnecessary destination parameter on the action link, but I'm reclassifying the issue as a Feature Request anyway so it won't cause more confusion.

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

The parts of views.theme.inc that are being modified by the patch were changed by #2977950: Move the bc layer for views UI CSS classes from views to stable9 → to remove the @todo. This needs to be rebased.

Although a test was added, the issue has expanded since then. Therefore, the test needs to be expanded to cover the additional special characters too.

Also, the issue summary needs to be updated with info about the additional special characters.

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

Are there uses cases where layout plugin derivers provide label or category?

The Dynamic Layouts module does. See https://git.drupalcode.org/project/dynamic_layouts/-/blob/8.x-1.x/src/Pl.... I know there are other similar modules out there too.

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

🐛 block--system-menu-block.html.twig relies on id attribute which might not be set Needs work was a duplicate of this issue. A little work was done on a patch, but this issue has way more work and discussion. Credit might be granted to those who worked on that issue.

I'm closing this as a duplicate of 🐛 Menu blocks in Layout Builder get malformed aria-labelledby attribute Needs work . The other issue is newer, but there has been a whole lot more work and discussion that's happened there.

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

I added an update path test.

dcam → created an issue.

I hid MR 58 because there was something wrong with it. I think it was caused by me opening the MR while the issue version was set to 3.0.2. MR 59 was properly created from the 3.1.x branch.

dcam → changed the visibility of the branch 3517066-process-dates-hook to hidden.

dcam → created an issue.

I verified that hook_fullcalendar_process_dates_alter() is non-functional. In the D7 versions this alter hook was invoked in the theme.inc file. It looks like that file was removed and split up during the D8 upgrade. During that process the hook invocation was lost and forgotten.

I restored the invocation, but felt like I had to update its definition. This is a BC break, but BC is broken already and has been for years. I don't think it matters and shouldn't require a new major version. Anyway, the new hook definition works better with FullCalendar::prepareEvent() because I can just plug in existing variables. I think that the context information delivers the same information that it did in D7, just maybe not quite as conveniently, i.e. the field info would have to be extracted from the fields array.

Anyway, I was able to successfully create a hook implementation with the code I've provided in the MR.

I'm setting the priority to Major since this bug renders the hook unusable with no workaround.

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

I implemented the prescribed fix per the HTML5 standard. The FunctionalJavascript test was reverted in favor of Unit test cases in the existing TextareaTest.

Updated the issue summary with the appropriate solution.

At least it's a unit test so the overhead is low.

Sorry, should have set it to NR before instead of back to RTBC.

I followed the instructions to revert my earlier changes and add the ignore directive.

The Coder module follow-up has been created at 🐛 False positive from Drupal.Commenting.InlineComment.DocBlock Active .

I figure that the Core queue follow-up should wait until this one is committed, but I can make it now if someone wants.

dcam → created an issue.

Rebased.

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.