🇪🇸Spain @penyaskito

Might not be required ATM, but will make things easier when/if config schema validation is strictly applied (in config schema is defined as boolean, and not as nullable).

The patch at #6 looks good to me but:
- Needs an update hook for setting the value while keeping BC (default should be the textbox value for existing installations)
- Needs to modify default config? (IMHO default should be the existing scaffolding robots.txt)
- Needs tests
- Needs upgrade tests?

I also think this would be a useful feature.

Our usecase is that we have the default robots.txt, but we want to alter it with the xmlsitemap integration that is already built-in here.
But if there are core/scaffolding changes to it, we would like to get those.
If we want to do that, we need custom code instead of using this module, which I'm guessing others might be doing too.

mikelutz → credited penyaskito → .

I need to debug this, but tbh might be a limitation of using "/" as the way to traverse the arrays in migrate itself.
But a warning at least would be useful.

griffynh → credited penyaskito → .

Just created 🐛 Allow "/" in headers Needs review which might be related, or even fixed with this fix.

penyaskito → created an issue.

@WimLeers this should probably closed as duplicated, but do you think is worth opening another issue for trying to reuse the symfony validators OOTB. specially for those like AtLeastOneOf?

znerol → credited penyaskito → .

LGTM thanks!

I can reproduce this issue with the Storybook module integration, the patch fixes it, it includes tests, and test-only run is failing.
Code looks good.

The only thing I could suggest is a test with two elements nested instead of one, to check that those are traversed from bottom to top, but that might actually be testing twig itself and not our code.

penyaskito → created an issue.

@nod_ We come from cl_components which allowed that, which might reinforce your argument that it's a door hard to close.
I don't see how @lauriii's proposal couldn't work with the few cases where the template is significantly different and looks like the sanest thing at this point.

Also @e0ipso already raised than having our own twig token parser in core is something he doesn't feel comfortable with. I would be happy to avoid writing that too :sweat

@nod_ My main goal is having separate templates, and so far looks like that's the most complex part of it. I'm scared we might end up with a solution that prevents adding that later because of BC. Could we split this to a different issue for easiness of review and getting it in? Definitely. Should we do that before we have the templates covered? IMHO no.

This needs sanity checks from @e0ipso.

Back to my original PR.

Like attributes, variant is a "glorified" prop. [...] Why not injecting the value as a prop instead of inventing a new notation (like we already do for attributes)?

The difference here is that attributes will be used on the template, while variant intends to modify the template being used.
I gave another try at this, and unless we alter the twig EmbedNode object for ensuring it has that context (and then we need to modify the EmbedTokenParser class, definitely low level code), it's too late for that.

> duplicating the variants (defined apart from the other props in the YML) as a variant (string, enum) prop during the component definition loading in the component manager

A new approach for this at https://git.drupalcode.org/project/drupal/-/merge_requests/8197

griffynh → credited penyaskito → .

penyaskito → created an issue.

Wondering if this could be a . It could be a default dashboard that the user is invited to delete when they are done. →

I've pushed my WIP for availability, but that's the blocked road.

I was working on this preceding Mateu's comment and reached that blocker.
If we use the same component id per variant in plugin manager, we reach a point where the variant name is lost.
Also, the template public prop is a show stopper for BC.

Had another chat with Mateu today.

An alternative sub-optimal implementation would be:

- Generate a different component id per variant on plugin manager (as if they were derivatives).
- Have some kind of "base component id" that we could use when we want to list components so they aren't repeated (API addition, should be BC)
- Have method on plugin manager for listing components (excluding variants), and a separate one for the loading (with variants, would be the existing one). This should be BC, but would allow Storybook and like to list them without "duplicates".

I think that would be still doable, but far from ideal. Otherwise this might be a won't fix.

penyaskito → created an issue.

I'd see a config action for this, but how do we deal with region names? Even if there are some unwritten conventions, you cannot ensure the default theme will have the region you expect.

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

Thanks!

Now that we have tests, this should be good to be merged if tests still pass.

All green!

Postponed on 📌 Provide config schema Needs review .

Started working on this, but faced some issues.
First, remote_media tests aren't updated for D10.
When I fixed this locally for using MediaRemoteFunctionalJavascriptTestBase, got some random error when trying to save the form from media_library. Found out that actually media_library tests are skipped in core, which might be a cause. See 📌 Skip Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest Fixed .
Still need to figure out if we can fix that.

penyaskito → created an issue.

penyaskito → created an issue.

Tests are failing with 10.x.

I think all the changes required are related to classy being removed.

Haven't tested this yet but codewise looks perfect to me.

penyaskito → created an issue.

Created new PR with the computed property approach.
Needs tests, but ready for discussion.

penyaskito → changed the visibility of the branch 3447203-improve-dx-of to hidden.

@larowlan suggested on slack:

"
So I think a computed property on list item base makes sense
Like we have processed on text item and date on date item
"

I get this as him not buying my initial implementation (which I neither was very happy with), but buying the idea. I'll be working in that ASAP.

penyaskito → created an issue.

Gave it a try and I think I ended up in the same place I ended up on Wim's computer.

Created https://git.drupalcode.org/issue/drupal-3440975/-/merge_requests/1/diffs for not polluting the original MR. I got stuck when in some place we are expecting typed data instead of the underlying value (or the opposite). Might be easier if we just extend from the symfony validators (as Regex does already), but definitely not as clean :-/

Discussed with Wim and we were looking at https://symfony.com/doc/current/reference/constraints/AtLeastOneOf.html, as hostname could be a hostname, a ipv4 or a ipv6. After some pairing we figured out that using composite constraints might be complex and could be deferred to a follow-up, as it's not very common to use a IP for a mail server hostname.

However, back at home just discovered the Symfony "Hostname" validator will consider "default" as an invalid one, and that's our default for e.g. when using sendmail, so we really need to figure this out here.

Retroactively tagging Portland2024.

Retroactively tagging Portland2024.

Retroactively tagging Portland2024.

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

penyaskito → created an issue.

penyaskito → created an issue.

penyaskito → created an issue.

penyaskito → created an issue.

Let's do that in follow-ups.

Not sure if we would want to fix cspell, phpstan et al issues here, or should we improve separately.

This is HUGE Fran. Didn't check everything, but all the links mentioned in this issue that I've checked are working as expected.

Thanks for your detailed answer. I need to mature this a bit, but wondering if we could decouple those.
My usecase is: I want editors to create simple quizzes (probably only multianswer questions), and people (in my case anonymous, but don't think is relevant) would complete them and see how they did at the end.

Multianswer answers being paragraphs is quite a lot, but I understand the reasoning. Could be simpler though.
I won't have "evaluator" feedback (so paragraphs, rules and their dependencies are overkill for me).
I need feedback to be custom based on results (so range should be in I think).
I don't want to export the results (no views_data_export, rest, csv_serialization). Actually I was exploring how we could swap the quiz results storage, because I don't even want to store them if possible, even I understand we need those while the users are completing the quiz.

It would be a pity ending with a custom solution or a mostly-duplicated "simplequiz" module because of those constraints.

👍 I wasn't sure I understood the problem, but @minirobot was patient with me and we huddled about it. Let's get this in :-D

Could we have a diff of both upgrade functions? Hard to spot what was wrong, and how this is fixed now.

https://www.drupal.org/project/drupal/issues/3438895#comment-15574276 ✨ Add the new Navigation to core as an Experimental module Fixed

It landed on 10.3.x, so we shouldn't need the tugboat magic.

Also maybe using 10.3.x makes tests pass again, which I'm not sure why they would fail here.

Trivial, no need to review.

Will be reverted in 🐛 Dashboard should build on top of the new experience builder initiative (revert ed2d2e6c) Active

penyaskito → created an issue.

Merged for the glory. THANKS A TON.

Updating credit

penyaskito → created an issue.

w00t

This patch will be required for core until we figure out 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active .

Navigation will face the same issue: 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active . So definitely not a dashboard issue, but a LB one that we need to explore together.

This isn't specific to Navigation but affects everywhere using layout builder, which now will be more places :-D

We had 📌 [Policy] Research about best way to limit the selectable blocks on dashboards Needs review for investigating this. We definitely need a combined approach.

Oh great! <3
I tested today (with the core MR) and didn't see that. Closing this unless I find^H^H^H
I was testing with umami, where administration is configured to be English for the admin (I worked on that :facepalm)

THIS LOOKS GREAT!

Added ✨ For RTL languages, Navigation menu should show on the right Active . IMHO is a "must" for stable, but we would want to check with RTL languages' native speakers.

penyaskito → created an issue.

@smurtgrave You said you reproduced it, could you add concrete steps to reproduce?
I could too, but only when having a custom field with custom validation, which is probably a lot of work and overkill for a test here. Maybe your STR are more simple?

penyaskito → created an issue.

I don't think that this could be feasible and support Mike's case unless the Appearance setting "Use the administration theme when editing or creating content" has more granularity (e.g. per entity_type/bundle)

This might need more guidance from @larowlan on his intentions.

AFAIK if we remove the check from AssetDumper, UrlHelper::compressQueryParameter and UrlHelper::uncompressQueryParameter would need to be updated too.

Just committed myself, so RTBC if green.

As I couldn't reproduce this anymore (#15), I at least verified that everything still worked with this applied (had to apply unreleased 🐛 Skip query string compression if zlib extension isn't available Fixed before so it applied), and it works.

I would RTBC, but there's a phpcs complain. Just left a suggestion.

Re-titled so title is up-to-date with what we are actually doing here.

penyaskito → created an issue.

I think this makes everything safer, has test coverage for the new behavior, so RTBC.

After a second read at our issue tracker: not everyone could reproduce this locally (even with prod settings -redis, enforcing caching- enabled), and we could only reproduce in production and not on test environments with the same settings. So tbf I might be wrong on my bet it wasn't caching after all.

@catch Before testing the MR, I tested just removing our revert and it worked this time. We had video proof and detailed steps to reproduce (this hit production), so I'm pretty sure I'm testing it correctly.

Luckily we also logged the urls at that time. Not going to share here to avoid any leakages, but it had page_state[libraries] uncompressed.
If I repeat the same operations now I get a compressed page_state[libraries]

We clear caches on our sync process, so I'd bet it's not coming from caching.

You mentioned 🐛 Pagination not working correctly in AJAX view with exposed filters Needs review , and it's our case. This happened with inline entity browsers that embedded views where ajax paging and exposed filters were enabled. Both paging or trying to filter by a search term failed (independently, not necessarily combined).

I've looked at your MR and even if I can't reproduce now, I think it's still worth to be on the safe side and merge it.

penyaskito → created an issue.

Completed the upgrade and tested it manually.
Do we expect an upgrade test too? If not this is ready for reviews.

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