#Needs Review Queue Initiative

I think this is good background reading for the other issue, but let's explicitly postpone it on that one.

Yes, extending existing Render Elements classes (as proposed in 📌 Slowly, very slowly start OOPifying the render system Needs review ) looks better than adding a Builder class for each Render Element as proposed in the current MR.

Also, when we will replace most (but not all 😉) of Render & Form elements by SDC, all the logic to remove will be contained in a single file.

Just leaving a note, in case someone else also who simply want to translate a string finds this issue. Because without the patch, in a view which has results, in order to get Twig to render a string in a "Global: Text area" in the "Header" section in Drupal 10, I need to enable "Use replacement tokens from the first row" and only then is a string such as {{ ('My string.'|trans) }} translated.

I spent quite some time working on making just 1 conversion, I agree with #153, this is not the way to go about it. 📌 Slowly, very slowly start OOPifying the render system Needs review is a much nicer solution.

I tried #142 but the amount of methods you'd need to add is staggering and at the end of the day since people use arbitrary properties in render arrays you would need to support some sort of set/get anyways so I leaned heavily on that and so 📌 Slowly, very slowly start OOPifying the render system Needs review is born. If that issue gets accepted this issue might be outdated.

Had a look through tests and I think this will be very hard to cover without being flakey.

AFAICT it needs to be in a JS test to do the draggable stuff (reparenting isn't supported on the menu form without javascript). We have a functional test in MenuLinkReorderTest which does almost nothing, and the existing JS test coverage only tests contextual links.

Reproduced this and the patch from #50 fixed it nicely. Rolling into an MR, still needs tests.

Automatically closed - issue fixed for 2 weeks with no activity.

Patch in #12 only changes a Drupal core file, while the patch in #2 also changes some of this theme's files. The latter can be used for the merge request.

Hopefully this is good enough.
I've rewritten everything with new approach.
Hopefully no one will be mad, this issue were not going anywhere anyway.

IS still needs to be updated

Following up if anyone can update IS with steps, if no follow up could close out in 3 months.

For instance if your password is mysql://user:pass#pass@host/db it should become `mysql://user:pass%23pass@host/db` to become valid url

Strange this is not fixed even in Drupal 11, and there not a lot of people complaining.

I can confirm this is still present, my password contains '#'. If I leave it like that, url is invalid, and if encode password it is not properly decoded.

You can encode password with urlencode (php function) or just find online url encoder - encode password only.

Attaching patch fix, that will decode url properly. There are a lot of comments telling about this issue on php.net parse_url page.

Also I think I covered #15

Although, when using a custom field formatter for file fields it's not clear how to set this option within the field formatter. The link URL object is generated in the preprocess function, and there's no way to pass additional attributes to the link element, only the warapper element.

Confirming #60 by setting the target for the link to _blank, which was my main use for this. I can't comment on anyone else's requirements.

function MY_THEME_preprocess_file_link(&$variables): void {
  $attributes = $variables['link']['#url']->getOption('attributes');
  $attributes['target'] = '_blank';
  $variables['link']['#url']->setOption('attributes', $attributes);  
}

Screenshots appear to be the same but would need to be part of the summary

I also misunderstood this ticket so apologize @prashant.c and @dcam.

So this is actually pretty straight forward and may not need the test coverage. Lets see what the committers think.

I'm a bit unclear what the specific issue is now.

I worked with @arla in 2016 when we did encounter this, but things have changed since then. We did deprecate and remove Drupal::l() and replace it with a Link::toRenderable(), so you should be able to alter this in additional preprocess functions as it is an array and not just a string now.

So it is possible to *alter* file_link template and its link structure and attributes. The thing I see is that it might be difficult to get enough context, for some cases you might want to alter only specific fields or so, then you'd want to alter the render array before this bit is rendered.

Additionally to the title, the issue summary also needs to be updated with the proposed solution and use cases why it's useful.

Looking good!

Reviewed merge request #12272 - diff is identical to the 11.x commit, pipeline is green, so I believe this is RTBC. Setting version accordingly.

I believe this should also be ported to the 10.5.x. and 10.6.x once ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review is committed on those branches.

Have tried to see where this is at.

The comment in #99 links to the 10.5.x pipeline which we can see is broken - as far as I can tell the 10.4.x pipeline from the same commit is https://git.drupalcode.org/project/drupal/-/pipelines/358805 which did fail but looks like an unrelated failure.

I've reapplied the commit from 11.x to the 10.4.x and it applies cleanly and appears to still pass https://git.drupalcode.org/project/drupal/-/merge_requests/12272/pipelines

I've done the same for 10.5.x here and its failing with what was reported before it was reverted https://git.drupalcode.org/issue/drupal-3278759/-/jobs/5409866#L412

The reason appears to be what was outlined here in #91 - except that the related issue ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review was committed to 10.4.x but does not appear to be on 10.5.x. That issue was closed as fixed - but my assumption here (have not tested) is that if that issue is backported to 10.5.x then this should pass.

I'm a bit confused on the timeline of when branches were open - but should this be postposed on ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review - and should ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review be reopened so that it can be applied to 10.5.x? Seems very unexpected that we have something in 10.4.x but not in 10.5.x?

ericgsmith → changed the visibility of the branch 3278759-10.4.x to hidden.

ericgsmith → changed the visibility of the branch 3278759-10.4.x to active.

Thanks for working on this!

Test failures seem related to this change.

Failing test

I'm also getting a similar error, but on a different occasion, for example when a (malicious) request goes to /contact/Perú
I'll try to reproduce it on a clean install and upload a failing test

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1267 Illegal mix of collations (ascii_general_ci,IMPLICIT) and (utf8mb4_general_ci,COERCIBLE) for operation '=': SELECT "name", "data" FROM "config" WHERE "collection" = :collection AND "name" IN ( :names__0 ); Array ( [:collection] => [:names__0] => contact.form.Perú ) in Drupal\Core\Config\DatabaseStorage->readMultiple() (line 111 of web/core/lib/Drupal/Core/Config/DatabaseStorage.php).

- Updated a new kernel test to validate the entity query results.
- Using other general practices for creating fields etc.

Code is already fixed so no changes are required in EntityReference handler plugin.
Hiding existing patches in favour of MR.

Tests are passing on local.

As per #48 this is no longer required

- Added a test to validate that attribute passed is exist on the render.
- Variable hint in the twig template.

Since there hasn't been a follow up in over a year going to say maybe this wasn't a needed task. Going to close but if still valid please re-open.

smustgrave → closed merge request !5690

I'm -1 on radio buttons. We're also introducing a new term "normal operation". I think "Enable maintenance mode" for the single checkbox would be perfect, and reads slightly better than "Put site into maintenance mode" to me. I don't believe that people struggle with the concept of checking or unchecking a checkbox.