🇧🇪Belgium @Wim Leers

Let's first get 📌 Add GitLab CI template Needs review in, so that we can observe the tests passing on Drupal 11 and require tests to pass on "the next major"? 😊

Nice start! :)

Being entirely new to this space and trying to review:

1. WOW — epic work! 😮👏
2. I found the issue summary fairly clear at a high level 👍, although kinda sparse 🫣. A new json:normal database Schema API data type makes sense. 👍
3. … but what does not make sense (to me, yet) is that the issue summary talks about how this is not for the field level, yet this issue's MR is introducing two new (test-only) field types: see the JsonBackedPropertiesItem and MappedPropertiesItem classes. That seems a contradiction at first sight? I suspect the point is to show how to better achieve the equivalent of https://www.drupal.org/project/json_field → with these new core APIs? If so: great! But could you then also explain/show why this is better? Is it better because less code is needed? Fewer hacks? Deeper integration?
4. Related: the issue summary uses the term "data type" many times, but approaching this issue with an Entity/Field/Typed Data API mindset, that reads like a new Typed Data DataType plugin, for a new json data type. Can you please disambiguate it in the issue summary and explicitly mention how it's different?
5. I find the current change records to be too sparse to help me understand the before vs after.
6. Could you please add a concrete use case to https://www.drupal.org/node/3389683 → ? And perhaps you could even add such an index on extracted_value for MappedPropertiesItem? That'd make it much clearer! 😄
7. Could you please add concrete examples to https://www.drupal.org/node/3389682 → , especially for the new ::jsonCondition()? (Which is also not mentioned in the issue summary?) 🫣

AFAICT this will block ✨ JSON-based data storage proposal for component-based page building Active , which is how I ended up reviewing this :)

Lots of comment nits plus a few actual questions. I think the change record should also be updated to be more explicit about what we expect field type maintainers to do: what are the updated best practices? (It captures that in a brief sentence or two, I'd like to see a concrete and , because that'll make it MUCH more likely that the ecosystem will adopt the new best practices: more explicitness.)

Totally! :)

Interesting, @pdureau, I'd definitely love to see that! I defer to @jessebaker, @bnjmnm, @effulgentsia and @lauriii to decide whether that's a direction we should pursue.

+1 for the principle. But I'd like to not yet deal with the full set of required boilerplate docs while the PHP code is still heavily in flux/PoC quality. (I copy/pasted the phpcs.xml of my CDN module because I knew that one already had some of these exclusions. Should've thought of this — but hey, at least things are already more consistent than before 😄)

IOW: I want to refer to <ruleset name="drupal_core"> without using core/phpcs.xml.dist wholesale.

Trying to make that happen…

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

I thought Recipes were intended to be only additive?

Let's get a PoC of this proposal working. Without the hashing parts for now, for the reasons @catch outlined in #9 and reiterated in #12, as suggested by @alexpott in #14. Let's tackle #2770417: Revision garbage collection and/or compression → instead.

I am building a PoC that does not use anything like field_union as @catch suggested in #15 (search for the line in his comment). There's no "field union config entities" at all, not tied to the component level or anywhere else. (See my comment #17. To expand on the last sentence of that bullet: the same component can have its 3 props A, B and C fulfilled by either 3 statically defined values that would need one field union, or 2 + 1 dynamically retrieved one, where there's 3 possible such pairs. That's far too many field union config entities to be practical. @catch did touch on this in #18, where he mentions 👍)

@catch in #18: RE: my confusing comment 🫣 — all I meant to say was that it's a problem we'd have to deal with eventually, but I don't think we should deal with it here.

@apmsooner in #22:

• That's simple to say as long as one doesn't need to worry about translatability, configurability or extensibility.
• This is true, and is why I think field_union is a mismatch for Drupal's Experience Builder (announcement links in #20).

@amateescu in #24: I like how simple that sounds! 😊 But then I realized that tracking the deltas of each of these fields would become fairly complex/brittle to keep "routing"/mapping individual deltas to distinct components (destinations), unless we'd be able to use arbitrary keys as deltas (then we could just put UUIDs in there). But \Drupal\Core\Field\FieldItemList::setValue() enforces the use of numeric keys.

#27: because base fields are guaranteed to exist for all bundles of an entity type, right down to the code level. Fields defined in configuration may or may not continue to exist, and may or may not exist for all bundles of an entity type. There is only a single table per field for all bundles of an entity type though — so there could've been even more!

@ctrlADel in #28: — something along these lines is what we're thinking of doing for https://www.drupal.org/project/experience_builder → , details TBD. But before we get to that point, we need to get the even lower level data flow and storage fundamentals figured out first. This issue covers a subset of those fundamentals. That's also where what we discussed at DrupalCon Portland and which you opened 📌 Create an example set of SDC components Active for is very valuable: a set of more complex components that nest components to ensure that we get those things working too.

@mglaman in #29: I had the same thought! 😄👍

Based on the input here so far and the very early stages this is in, I made some pragmatic decisions:

1. set up CODEOWNERS to encourage different people owning different areas. For example, @hooroomoo has built Vite-powered hot module reloading, so I marked them as the owner. @bnjmnm and @jessebaker felt strongly about using Cypress instead of Nightwatch for functional testing of the UI, so they own that. I built the data model pieces so far, so I currently own that.
   I can imagine that @larowlan would want to own the JSON:API support given the strong vision he articulated in https://www.previousnext.com.au/blog/pitchburgh-diaries-decoupled-layout....
   And so on. We can get more granular. We can evolve this. The goal is to balance speed (fast!) with cohesion (few owners of each piece).
2. PHPCS is now passing and required: https://git.drupalcode.org/project/experience_builder/-/merge_requests/3 — albeit with some rules relaxed — no need to document in magnificent detail something that is not at all precisely defined/nowhere near its final shape :)
3. PHPStan is now passing and required, and is configured to level 8: https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...
4. @bnjmnm is working on Cypress CI: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5
5. @jessebaker is establishing the front-end pieces: stylelint, Radix UI (for now, per @lauriii's recommendation) and much more importantly the outlines of the JS app powering the UI: https://git.drupalcode.org/project/experience_builder/-/merge_requests/8

Repository structure established ✅

Much, much more to still be figured out!

First: thanks for opening this issue! 😄

Do you know about https://www.drupal.org/project/jsx → ?

@bnjmnm has built a working PoC for this, with Drupal-defined (Form API-defined) field widgets being rendered — unmodified! — in React. See https://git.drupalcode.org/project/experience_builder/-/commits/eb-form-....

IIRC there also was a demo of the JSX theme engine rendering Twig and React components interspersed — a tree of both React components and Twig templates, with no restrictions on which is where.

Assigning to him to respond here :) Would be a shame to duplicate work.

Once setting level 2, you run into lots of Prophecy-related errors. Same thing in Drupal core: 📌 Bump PHPStan rule level to 2 Active .

👏 👀

Wim Leers → created an issue.

@Driskell Could you please open a new issue? 🙏 The functionality was restored to be AFAICT identical to how Drupal core does it … but if there's problems with that approach, we should figure that out in a new issue.

I ran into this today and was shocked was not already the case 😅 📌 [11.x] [policy] Require MySQL 8.0 (increased from 5.7) or MariaDB 10.5 (increased from 10.3) for Drupal 11 Active landed 3 months ago! We must've overlooked this 😬

Wim Leers → created an issue.

I tried to get this to work but ran into some challenges. I get that there are a bunch of @todos such as

#      $ref: "json-schema-definitions://experience_builder_example_components.module/two_column"
      # @todo refs don't seem to work yet

(Yep, that only works in the https://git.drupalcode.org/project/experience_builder/-/tree/research__d... branch right now!)

But I did expect that I'd be able to kinda get this branch to work, since you didn't share any caveats/instructions on how to get it to work 😅

This seems to be triggering a lot of SDC schema validation errors. I spent a good while stepping through the SDC schema validation process and fixed a few — the first one:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("We found an unexpected slot that is not declared: [column_one, column_two]. Declare them in "side_by_side.component.yml"."). in Twig\Environment->compileSource() (line 2 of modules/contrib/experience_builder/modules/experience_builder_example_components/components/compound/side_by_side/side_by_side.twig).

required a work-around that I think goes against the intent of what you were trying to convey.

The second change (a required label prop that AFAICT should've been optional) seems just a minor oversight.

But then the third:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [width] String value found, but an object is required/n[width] Does not have a value in the enumeration [25,33,50,66,75] in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

had me to step through a significant part of the schema validation process to make sense of what was going on.

So … rather than me spending multiple hours trying to figure this out while being uncertain of the intended direction … could you perhaps provide some additional context, or just instructions on how to see the whole thing in action? 😇🙏

EDIT: cross-posted with #4 — yeah, I definitely don't get to see that point 😅

@lauriii makes a very good point — when I wrote #4 (in between DrupalCon discussions) I was not thinking of the fact that we want to make it available for any site to install as a contrib module 🙈

So my "I'm sold" comment was based on the wrong premise.

@larowlan & @alexpott: I think that means it probably makes sense this project does not follow Recipes' example.

(Perhaps we can land on a hybrid approach, where we transform the contrib module to a modified core at test time, to piggyback on the entire core testing infrastructure: run only this module's test suite, but do so with the module at core/modules/experience_builder. Not sure yet if that's actually better.)

WOWWWWWWWWWWWWWWWWWWWW!!!!!!!!!!!!!!!!!

We all owe @kim.pepper thanks, beers, lemonades, applause and gratitude for many years to come, because thanks to his unrelenting effort Drupal is now finally consistent in this very security-sensitive area! 🤩🥳

📌 [PP-4] Unify file upload logic of REST and JSON:API Postponed is done! 🤯

I believe that means we can close this?!

🤩

I'm sold.

Reproduced. 😬

👀 Contrib should be able to gradually opt in, this is unintentional disruption. Apologies 🫣 Investigating…

Thanks for making this happen, @bnjmnm! 🤩🚀

Excellent start! Thanks for helping out making all of Drupal core's config validatable! I left some pointers in my review on the merge request 😊

YAY! Welcome, @mikelutz! 😄🥳 Unfortunately GitLab seems to just have gone down!?! 😱 I'm getting 503s!

#2: Hah! :D

This is a good start, but there's a bunch left to do.

Nice progress here! 😄 Still needs some work, but for each of the things that still need to be addressed, there's prior art/patterns to look at 😊

I don't see any justification for making is_admin and weight optional values? I kinda get weight being optional … but not is_admin.

Plus, if they're optional, then that should be reflected on the class properties too, and that's not the case:

  /**
   * The weight of this role in administrative listings.
   *
   * @var int
   */
  protected $weight;

+

  /**
   * An indicator whether the role has all permissions.
   *
   * @var bool
   */
  protected $is_admin;

#16: indeed, PluginExists is what you want.

Most importantly: you set this to but I see no new commits? 😅

Alright — thanks!

https://www.drupal.org/project/drupal/releases/11.0.0-alpha1 → is out!

Re-running tests, then merging and tagging 5.0.0-alpha1 🤓

Hmm … odd. I’ll try to reproduce this.

Can you share your cdn.settings but with a dummy domain name? 🙏

Thanks for reporting!

#7: Yes, it would. And along with that, it'd need a validation constraint that treats null as invalid if >=1 theme is installed.

I think this is what @alexpott was asking for — until #3135592: Cannot implement a custom user cancellation method → and/or ✨ Provide a Entity Handler for user cancelation Needs work happen, a truly elegant approach won't be possible anyway 👍

Drupal has the concept of a default country, and reading this config file is the only way to access it.

Hm, good point! 😬

What path forward do you propose?

📌 Improve JSON:API test failure messages to include errors when data is expected Fixed landed today and conflicted. Resolved.

I will do a live demo of this in my DrupalCon Portland talk next Monday. 🤓 So here's a backport to 10.3.x (against 0da9964174175a013f8d517cdee1ce08e590f440).

Let's wait for the first Drupal 11 release.

#42: I don't think that's entirely accurate; that module does something rather special: #3173772-15: TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given → — it uses core config as if it was its own config (with schema it controls). I do not think that is a widespread practice?

If anything, after this (somewhat painful — although the changes in the module seem trivial) transition, any module doing something like this will have stronger guarantees after the value is being strictly validated: they'll know much more precisely what data to expect.

Found this thanks to @Berdir's comment at #3437325-42: Add validation constraints to system.date → .

Interesting:

[error]  TypeError: Argument 1 passed to Drupal\commerce\Country::__construct() must be of the type string, null given, called in /var/www/html/web/modules/contrib/commerce/src/Resolver/DefaultCountryResolver.php on line 35 in Drupal\commerce\Country->__construct() (line 23 of /var/www/html/web/modules/contrib/commerce/src/Country.php) #0 

👆 None of that code is in Drupal core: neither the Country class nor the DefaultCountryResolver class.

It seems that the problem here is that this module has built infrastructure that assumes a certain data shape ("strings only"), but it does not control the shape: Drupal core does.

Because of that (fine!) choice, this module's infrastructure should adhere to https://en.wikipedia.org/wiki/Robustness_principle: — in this case it probably should detect whether the data is anything else than a 2-character string, and if so, fall back to the empty string (which this infra has assumed to be the fallback value so far).

Two examples of prepping for Drupal 11:

1. plugins converted from annotations to attributes: https://www.drupal.org/node/3229001 →
2. static data providers: https://www.drupal.org/node/3365413 →

Wim Leers → created an issue.

I agree the schema looks correct. 🤔

However, the Config Inspector module does not contain the string missing schema anywhere. So it must come from \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue() which contains:

    if ($element instanceof Undefined) {
      return [$error_key => 'missing schema'];
    }

1. Can you please provide a config export of the config that triggers this error? Just paste the YAML here :) I bet that's where the problem is: the schema looks fine, but probably the config data itself is not.
2. You can do what I'd do too: put a breakpoint on the quoted line above and re-trigger the error, then observe how you get to that point!

Yay! :)

This is both more thorough than the validation ever was and friendlier for the end user!

I'm satisfied with where this has arrived.

(I'm less satisfied with the anticipation of complex CDN configurations that likely nobody ever used … and now I'm carrying forward the internal complexity for that 😅)

1. There's one last thing to address: https://git.drupalcode.org/project/drupal/-/merge_requests/7321#note_294486 :)
2. The MR needs to be updated to target 11.x.

Responded to ~half a dozen threads.

And did a review pass myself too over the parts I had not seen before. Two questions I have:

1. changing InstallCommand looks like a BC break? https://git.drupalcode.org/project/drupal/-/merge_requests/7410#note_306489
2. why do we need all the revision_translation_affected in the default content YAML if it's a read-only field? https://git.drupalcode.org/project/drupal/-/merge_requests/7410#note_306499

I'd expect content entity validation to detect this?

Agreed that the source of truth should always be a JSON schema definition.

But I think @effulgentsia was merely stating that it's possible to provide a TypeScript-only DX that generates such a JSON schema definition. I doubt he's proposing to enforce it, nor make it the default.

The fact that it's possible is … nice, interesting and potentially powerful eventually :)

"the other MR" == ✨ Allow CSS to be added at end of page Active ?

Looks like this MR confirms my hunch at #3439017-5: Umami page.tpl.php breaks #create_placeholder for blocks → .

Did you mean core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig?

It's not obvious to me what the problem is. I'd bet it's

{% if page.pre_header|render|striptags|trim is not empty or

which appears to be a work-around for 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work ? 🫣

😨

Sorry for the oversight :(

🌱 5.x requiring PHP 8.2 and >=10.2, 6.x requiring PHP 8.3 and >=11? Or skip that 5.x? Active is in, rebasing on top of that, to start seeing the actual failures instead of out-of-scope D10-to-11 trivialities.

The two remaining PHPStan errors will be fixed by 📌 Adopt Drupal core 10.3 config validation infrastructure Active 👍

Per #3421351-8: 5.x to require PHP 8.3 and >=10.3: bump requirements + fix deprecations → : 10.3 added some crucial infrastructure, 10.2 alone does not get us far enough.

Review posted on MR — if I'd found nothing in my review, then I'd have done #9 for you — but since I can't RTBC this yet anyway, I'll let you handle #9 too 🤓

Tests are failing and I think we can fix that by just pushing the latest origin/11.x to this issue fork 🤞

To get this MR to pass tests, this issue must also address all deprecations. For example: PHPUnit has become more strict, 📌 Require `langcode: …` only for simple config that contains translatable values RTBC , and more.

The FullyValidatable validation constraint only became available in Drupal 10.3, see https://www.drupal.org/node/3404425 → .

So, 5.x should require 10.3, not 10.2.

Unsurprisingly: green :)

The 5.x branch now exists. So let's bump the requirements here.

Interestingly: no update path is needed here, because updating from Drupal 10 to 11 will uninstall the Tour module and hence also delete all its config. 👍

Per #4.

Especially now that tests are green for 4.x on 10.3, since 📌 CdnIntegrationTest::testCss() fails on Drupal >=10.3 since #3432183 Needs review .

Hah — just using a different image in the test is sufficient to work on Drupal 9, 10 and 11 all at once 😊

Wim Leers → created an issue.

🐛 Since 4.x release, hook_file_url_alter is no longer called, breaking several modules Active was fixed and will ship in the 4.1 release. Can you please test again and confirm that the problem is gone? 😊

@paulvb FYI: this commit will break your decorated file_url_generator service … but in doing so, it should have made yours obsolete.

So you should be able to:

1. delete your code
2. bump the version requirement from drupal/cdn:4.0 to drupal/cdn:4.1

😊

Tests failed as expected: https://git.drupalcode.org/project/cdn/-/jobs/1494115 👍

Fix just pushed, should be green. Will merge upon green.

I see the sibling issue in the Crop module has even fewer responses: only you and I, @Driskell, seem to be following it as of today 😅

That explains why so few people have reported this regression: it seems almost nothing in the Drupal ecosystem uses hook_file_url_alter() for anything besides serving from a CDN.

Looking at the code

/**
 * Implements hook_file_url_alter().
 */
function crop_file_url_alter(&$uri) {
  // Process only files that are stored in "styles" directory.
  if (strpos($uri, '/styles/') !== FALSE && preg_match('/\/styles\/(.*?)\/(.*?)\/(.+)/', $uri, $match)) {
    // Match image style, schema, file subdirectory and file name.
    // Get the image style ID.
    $image_style_id = $match[1];
    // Get the file path without query parameter.
    $parsed_uri = UrlHelper::parse($match[3]);
    // Get the file URI using parsed schema and file path.
    $file_uri = $match[2] . '://' . $parsed_uri['path'];

    // Prevent double hashing, if there is a hash argument already, do not add
    // it again.
    if (!empty($parsed_uri['query']['h'])) {
      return;
    }

    if ($crop = Crop::getCropFromImageStyleId($file_uri, $image_style_id)) {
      // Found a crop for this image, append a hash of it to the URL,
      // so that browsers reload the image and CDNs and proxies can be bypassed.
      $shortened_hash = substr(md5(implode($crop->position()) . implode($crop->anchor())), 0, 8);

      // If the URI has a schema and that is not http, https or data, convert
      // the URI to the external URL. Otherwise the appended query argument
      // will be encoded.
      // @see file_create_url()
      $scheme = StreamWrapperManager::getScheme($uri);
      if ($scheme && !in_array($scheme, ['http', 'https', 'data'])) {
        if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
          $uri = $wrapper->getExternalUrl();
        }
      }

      // Append either with a ? or a & if there are existing query arguments.
      if (strpos($uri, '?') === FALSE) {
        $uri .= '?h=' . $shortened_hash;
      }
      else {
        $uri .= '&h=' . $shortened_hash;
      }
    }
  }
}

— https://git.drupalcode.org/project/crop/-/blob/8.x-2.3/crop.module?ref_t...

… I do see one major problem:

      // If the URI has a schema and that is not http, https or data, convert
      // the URI to the external URL. Otherwise the appended query argument
      // will be encoded.
      // @see file_create_url()
      $scheme = StreamWrapperManager::getScheme($uri);
      if ($scheme && !in_array($scheme, ['http', 'https', 'data'])) {
        if ($wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($uri)) {
          $uri = $wrapper->getExternalUrl();
        }
      }

… that would mean that "cropped image" file URLs never actually would have been served from the CDN. But until ✨ Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) Needs work is supported, that wouldn't be possible either.

Conclusion: I'm still +1 to restoring this functionality, even though it seems there's only a single contrib module using it.

Which contrib/custom module?

OMG I'm so sorry, I was so very wrong in #3! 😱🫣 Clearly I explained too much in a hurry.

I was a bit shocked finding out that the cdn module decorates the core FileUrlGenerator and leaves out $this->moduleHandler->alter('file_url', $uri);...

I am too now! 😰

This is a major, major oversight on my part. But also of https://www.drupal.org/node/2940031 → .

You see, I'm the one who originally introduced hook_file_url_alter() into Drupal core ~15 years ago now: #499156: CDN integration: allow file URLs to be rewritten by hook_file_url_alter() → . Its original purpose was actually … literally to support CDNs. Not to do other file URL transformations.

But it's precisely those other file URL transformations that should still be supported through this hook, the change record I linked is only relevant for the "serve from CDN" use case! 😱 Escalating to critical…

Oops, I see the original MR's composer.json was slightly incomplete 😅