🇺🇸United States @mglaman

This has uncovered 🐛 Cannot delete a field which uses JSON type Active

Ready for some full reviews!

Should be set, now.

Found a flaw. The route experience_builder.experience_builder requires a saved entity. There isn't a "new entity" route yet. I don't know if this issue should be modifying \Drupal\experience_builder\Controller\ExperienceBuilderController::__invoke to allow $entity to be nullable.

'base' => \sprintf('xb/%s/%s', $entity->getEntityTypeId(), $entity->id()),

This would break if null. But the entity type is a route parameter, so it could be added as a method argument.

Then in the method, if the entity is null we could pass an ID of `0`?

The initial review was addressed

This is not done in that issue. This issue is to implement entity owner interface.

It also makes the standard way of doing "Who created the page" a lot easier (target default data table vs revisions)

The MR working on adding the entity type already extends Drupal\Core\Entity\EditorialContentEntityBase, so it'll be covered. But not the idea of "owning" a page like node and media already do

Technically the revision log gives us all of that information, revision log entries always reference a user.

This issue would enable "edit|delete own" and "edit|delete any" permissions.

mglaman → created an issue.

mglaman → changed the visibility of the branch 3487533-cannot-delete-a to hidden.

ugh picked the wrong branch

mglaman → created an issue.

Thanks! I appreciate helping keep dependency tree at a minimum

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

mglaman → created an issue.

Instead I think that this should not be rolled into the main XB module and should become a standalone submodule. Then sites can have either this module, or node.module, or both installed, as they see fit - but I don't see a reason that we should force this to be available if someone doesn't want it.

I will update For 1.0.x we may remove the content entity type... to be about moving the entity type into a submodule.

Assigning to handle loose threads.

I'm going to just delete most of the issue summary and point to the ADR. The issue summary was drafted before the ADR was created. I don't want to have to keep updating two sources.

Side note: I'm not the one doing the full development on this, so I respect your decision either way, but I guess the beauty of open source is being able to express your opinions and thoughts and try help in the ways you can :)

I appreciate it, because it forces us to reconsider all positions.

I'd argue this is a great reason why to do it! It's early development and you get to find these issues early. Doesn't mean you need to adress them early.

Honestly it is super draining. Especially when earlier evaluators who aren't technical find bugs. Now you're toiling on those bugs and edge cases. Or constantly documenting them, but no one reads documentation. We _could_ do in code workarounds to try and stop it (prevent X module install, etc.) But now we're doing even more code work.

Also, as we integrate this new page, whatever we do will make it work with any entity type. We're just not exposing that directly. Maybe we hardcode some items in the XB JavaScript to target "page" and it's field. But then it's a low effort ticket to populate this into drupalSettings so it's truly dynamic. Heck, maybe this exists. I haven't dove into the code entirely. I'm just starting, with this as my jump in point.

I don't really think this is a problem. You don't need to focus on solving those problems right away, but it's good to acknowledge them.

See above. We don't need to solve, but we will be inundated by support requests.

If you expect people being able to go into production during early development it probably makes sense to have it separated early :)

This is exactly it. A curated experience to try and evaluate the Experience Builder. Maybe not into production, but as part of the Drupal CMS demo or other semi-production evaluations.

Personally I think it's a great idea to try integrate it directly, because it would also allow time for existing integrations/systems to fix said issues from their end as well.

I'm jaded from my experience with Drupal major version upgrades and time with the Commerce ecosystem, but folks don't do this. It'll end up being up to the XB team to solve those problems.

Allow the option to revert back to the older + faster PECL php parser - possibly via a config or settings.php setting

It is, but not easily.

/**
 * Override the default yaml parser class.
 *
 * Provide a fully qualified class name here if you would like to provide an
 * alternate implementation YAML parser. The class must implement the
 * \Drupal\Component\Serialization\SerializationInterface interface.
 */
# $settings['yaml_parser_class'] = NULL;

The hard part is: The class must implement the \Drupal\Component\Serialization\SerializationInterface interface.

Marking for review, ADR in the MR.

I need to double check concerns from #10, #17, and #22 are in Consequences and have mitigation solutions, and that the following statement provides a to them if still problematic:

For 1.0.x we may remove the content entity type and opt for a locked content type that Experience Builder provides. This is acceptable, as we're in an early preview state with expected data loss between versions. Or we can create a migration from the content type type to the content type.

I do not have the bandwidth to work on this.

I do not have the bandwidth to work on this.

I was asked to open an ADR, https://git.drupalcode.org/project/experience_builder/-/merge_requests/390 based on the issue and discussions thus far.

But I think the decision should be based on to what extent do we think un-modeled pages are sufficiently distinct from modeled content, rather than on concerns that we won't be able to implement sufficient config import validation to prevent people from making config changes that will break the code.

Agreed, and that was the initial basis of my thoughts. I figure there will be. Then it just spiraled from there into focusing on arguments on technical tidbits since I knew it'd be controversial for most technical Drupal users.

mglaman → changed the visibility of the branch 3482259-research-landing-page to active.

mglaman → changed the visibility of the branch 3482259-research-landing-page to hidden.

phpdoc was correct, not native type hint. opened MR

mglaman → created an issue.

I need to read #22, but dumping some notes I forgot to put on Friday.

By using a custom entity type, all contrib which influence and effect nodes do not work. This can be problematic but also a blessing. And it prevents random contrib from breaking things due to us treating a specific node bundle as a snowflake. So that should be considered.

I'm becoming indifferent. I believe a new entity type makes it easier to work with and removes a lot of extra work before all entity types and bundles are supported in XB. But it seems like "make a third party settings and hack nodes" is the easier route in terms of debate and just getting started, now.

Good enough of an answer for me! Maybe that dangerous tags tidbit prevents it from running in the editor only 🤔

Let's definitely lock it down. Thanks for entertaining my questions.

will find the lack of HTML restrictions to be a problem

Totally agree. It's just, as you said, all anecdotal.

See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/edito... which defines dangerous tags and the limited HTML extends

I'm just curious what are the dangerous HTML tags folks are worried about. And if they even work if CKEditor doesn't have a provided plugin supporting them

When building the Acquia DAM CKEditor 5 integration, the allowed HTML setting seemed disconnected, as in it didn't matter unless a CKEditor plugin supported the tag.

I don't need to die on this hill. I'm just trying to call out that Drupal already has pretty safe limits already and most may not be aware.

I ask, because I'm pretty sure Drupal still prevents it. Has anyone tested? Once upon a time in D7 it was an absolute pain to allow iframe and script. Unfortunately that was an actual requirement

What are the security concerns? Even with full HTML I didn't think iframe or inline scripts are allowed

mglaman → created an issue.

      // Get the default provider.
      $default_provider = $this->aiProvider->getDefaultProviderForOperationType($operation_type);
      if (empty($default_provider['provider_id'])) {
        $this->messenger->addError($this->t('No AI provider is set for chat. Please configure one in the %ai_content_settings_link or setup a default Chat model in the %ai_settings_link.', [
          '%ai_content_settings_link' => Link::createFromRoute($this->t('AI Content settings'), 'ai_content.settings_form')->toString(),
          '%ai_settings_link' => Link::createFromRoute($this->t('AI settings'), 'ai.settings_form')->toString(),
        ]));
        throw new \exception('No AI provider is set for chat. Please configure one in the AI default settings or in the ai_content settings form.');
      }

Ah, hold on. I see "Moderation" is a setting which requires specifying a model and doesn't have a fallback. I think the problem is the error logged.

What's quirky is summarize text and text generation work fine.

mglaman → created an issue.

Thanks for the version bump! This should be good to merge

@eiriksm did you manually test it locally or no? That's our one gap in coverage. The tests are Node only and we need to write the Cypress tests for this still.

Either way, I think fine to merge as the trial is having issues already. Maybe this fixed them!

mglaman → created an issue.

Changed to this:

        if ($message->getRole() === 'system') {
          $system_message .= $message->getText();
          continue;
        }

It appends any chat messages with the "system" role into the system payload parameter for Messages API.

Opened an MR. But I'm curious:

- Is this a limitation of Bedrock or the model used?
- Should they be converted to act as stacked user messages?

I also hit this. I tried the following thinking it would work:

'role' => $message->getRole() === 'system' ? 'assistant' : 'user',

But then the response was:

A conversation must start with a user message. Try again with a conversation that starts with a user message.

Using the "mistral.mistral-large-2407-v1:0" model

So I patched as:

      foreach ($input->getMessages() as $key => $message) {
        if (!in_array($message->getRole(), ['user', 'assistant'])) {
          continue;
        }

Then it worked. A different/more efficient approach than OP.

mglaman → created an issue.

This made me realize also that just exposing the meta tags fields is not enough. We'd likely have to build something more custom for it because the meta tags field doesn't provide sufficient controls over meta tags – specifically it doesn't allow adding images. This obviously raises the question; what source should we use for the image, i.e. media (and which bundle) vs image field?

We should eyeball what the Drupal CMS SEO recipe has done for more information. @lauriii and I discussed that there would be a minimum of a title, description, and image field. Then Metatag can be configured to use these for most common purposes (like embedded links in social media.) We'd want to use a media field, though, versus just uploading one off images via the image field. We can limit the allowed media types based on their allowed source field type (image.)

We should also provide a default Metatag field so that Metatag works.

How would it impact our decision if we there was a chance that in future we may decide to allow adding additional view modes to these landing pages? Is this something contrib could enable if we decide to not support this but it's something that is needed in some cases?

View modes are not attached to Field UI / field configuration management. They're managed under "Structure" so they can be managed/designed via 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active when that is completed (regardless if standalone entity type or locked content type.) Maybe XB will allow creating view modes itself.

thanks so much @boulaffasae! assigning to myself, I'm going to fiddle around a little bit tomorrow

yeah, made a quick edit to verify the tests pass with it not tagged as an HTTP client middleware https://git.drupalcode.org/project/drupal_cms/-/merge_requests/161

If it passes we can delete the code entirely. But keep the trial module for in-Drupal-trial functionality later.

This should be fixed now that we have 📌 Bump php-wasm and php-cgi-wasm to 0.0.9-alpha-21 Active which fixes HTTP support. Setting to NW to verify

The naming problem is actually a good reason NOT to separate it!

The naming isn't the main reason.

We can protect against doing dumb things in the UI, e.g. you can't delete a content type if there are any instance of it.

My concern isn't the UI. We're expecting people to export the config this module introduces and keep it maintained. Which means someone can edit it and import it, and we're responsible for also updating it if need be. I'm biased here, so maybe someone else should have researched this. But I don't see why we should be forced to manage configuration when it can be managed in code. Especially if the goal is to provide a very specific and tailored experience in this use case.

But admittedly I don't understand all of the technical side of why it's hard to properly lock it.

We're going to write a bunch of specific code to deny access to operations and hope we did the right ones and then have to maintain these code paths. Which takes away from the main development tasks.

Whatever is decided, I believe the items in Entity implementation from the IS can be applied to a node type. We can use a bundle field definition in code to attach to our bundle without field config and provide a conditional view builder.

started some ideas on the entity implementation, hopefully shows how it makes it easier to get started with building

fix example screenshots

Thought of a few things:

• A custom entity type avoids naming conflicts with existing content types (machine names.)
• If folks don't want to use the unstructured page, they just don't use it – don't grant the permissions for usage – and use XB on nodes (which comes after pages)

We found out the "bug": php-wasm added pdo_postgres support but it is compiled statically, not as a shared library like the other pdo extensions. So Drupal thinks Postgres is available and that's why the interactive installer test broke. We just need to workaround this in the trial installer.

Forgot to follow up on the issue

Do you think executing batch directly in our .phpcode file would be much better ?

I think so, because as you've done we can bubble progress messages to the UI

Having a separate list for landing pages seems hard to justifying?

I'm curious why? You'd have to filter /admin/content to find them anyway, and in most CMS each type of content is usually its own page. I agree it set off my "Drupal-senses", but then I did comparisons with other CMS or CMS-adjacent products.

Going back to "Being able to create sorted lists of landing pages and structured pages", this is actually feasible with Search API and is a common pattern for mixed entity type result lists.

is also a Stronger con imo

Do folks often reference landing pages from regular content like this, as an entity reference? I'm not saying it shouldn't be allowed. I'm just curious. Usually its landing pages which reference specific types of content used in collections (events, articles, etc.) In my experience things like articles usually reference one specific type of thing "Related articles" or "Documentation" or "Suggested products." In this case it could be a direct reference to a landing page.

Added node-specific things to consider for LinkIt and LinkWidget

updating IS with name

The env value is respected. It's just that the form submission isn't working the same as it used to for some reason. The default value is sqlite but form validation has gone sideways.

Okay we're close. It looks like setting env for CGI isn't working in the latest alpha

Yes let's start with these two tasks before moving to the UI

- we need an "apply recipes" script, like the install and login script, which reads the recipes from the install parameters
- take the new pre installed test and run that script, verify recipe added

The PHP script looks right. Good call on instantiating and calling RecipeCommand directly. I believe Symfony provides an ArrayInput class so we can easily pass the input parameters. We can pass a BufferOutput (named something like that) to catch the output and verify the outcome. Or to catch progress to pass up to the UI

alpha24 is out and needs a try

Awesome, thanks so much. I think we could merge this MR but keep the page open to expose recipe selection from the UI.

Thoughts on next steps:

- we need an "apply recipes" script, like the install and login script, which reads the recipes from the install parameters
- take the new pre installed test and run that script, verify recipe added
- add to the UI a way to select the recipes, a hard coded list to start, which provides values added to install parameters
- add a new script which can find the available recipes and dynamically list them in the UI, I think this can be a follow up

I can help land this part unless you'd like to try, boulaffasae

quirk in vrnzo, seanmorris found out and is working on a fix

thanks so much boulaffasae! Pipeline is green, which means we're kept the existing tests in tact. It'd be great if we added a new test like `cgi-test` or something which used the install trial and verified the login script works

so it extracts and

await runPhpCode(php, rootPath + '/public/assets/login-admin.phpcode')
  const [, text] = await doRequest(phpCgi, '/cgi/drupal')
  assertOutput(cgiOut, 'GET /cgi/drupal 200')
  assertOutput(cgiErr, '')

  expect(text).toContain(`<title>Home | ${siteName}</title>`)

but without the install steps like the existing one

tried alpha23, failure: https://git.drupalcode.org/issue/drupal_cms-3483001/-/jobs/3154364#L82

⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯
TypeError: Headers.append: "" is an invalid header name.
 ❯ node:internal/deps/undici/undici:12618:11

I think it's this HTTP/2 bug in undici, maybe a node 18 issue? https://github.com/nodejs/undici/pull/2332 I'll have to run it locally on my node 22 to see

Since we hit a bug in vrzno on streams with HTTP method configured, I opened an MR which just bumps sqlite https://git.drupalcode.org/project/drupal_cms/-/merge_requests/149

and https://github.com/seanmorris/php-wasm/issues/67

still need to patch for https://github.com/seanmorris/php-wasm/issues/62 I believe

Yeah it should be that error. But at the same time, I was going to keep the interactive and automated install stuff, so that we can go back to different installation options for testing. I was thinking of having the build-trial command do this:

 find . -depth -type d -name tests -exec rm -r -f "{}" ';'
 
 # Create a trial archive without a database
 composer archive --dir=../public/assets --file=trial-uninstalled --format=zip
 
 # Install the site so that the trial is immediately available.
 vendor/bin/drush site:install --yes
 
 # Create the trial archive.
 composer archive --dir=../public/assets --file=trial --format=zip

Don’t add the new artifact to the GitLab artifacts, and it can be used locally only

The fact we're seeing that and not "database exists" error means something else isn't write. Because it's trying to install and the step is missing since the database configuration is already set. If the sqlite database was there, it'd error about it being installed already.

bbrala → credited mglaman → .

bbrala → credited mglaman → .

No we don't want that change, by post processing I mean a `sed` on the generated `settings.php`

The problem is that we're trying to not provide a settings.php file. I guess we could post-process the file and clean up the absolute path.