Next steps for this meta will involve around the in-XB navigation. Working on a doc and issue map. I'll update here once there are more next steps.
Personally I think we should leave the restriction in for now and open a follow-up to add test coverage.
Agreed. The main goal was to ensure we have two testable paths, as there is some upcoming work which will be made a lot easier by targeting pages only.
now f.mazeikis can review :) https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... should fix it
Ugh, I looked too fast:
self::assertArrayHasKey('xb_test_page', $this->drupalSettings);
This is from the test module hook xb_test_page_xb_page_view:
function xb_test_page_xb_page_view(array &$build): void {
$build['#attached']['drupalSettings']['xb_test_page'] = [
'foo' => 'Bar',
];
It's to verify \Drupal\experience_builder\Entity\PageViewBuilder::alterBuild. The hook needs to specify the library I guess.
Merged, I feel the power! 💪
Perfect, thanks for optimizing
I'm slightly confused since the XB controller specifies drupalSettings in its render array. I don't get why removing the library definitions would break the test...
Unless in Kernel test the automatic addition of the library doesn't happen like in a fully bootstrapped system? I'll dig in.
The code was:
$page = $this->entityTypeManager->getStorage('xb_page')->create([
'title' => $title,
'uuid' => $uuid,
'path' => [['alias' => $path]],
'status' => TRUE,
]);
$page->save();
Which is what a lot of developers may do, since most do not call `validate` before saving. I have a feeling people will programmatically create the entities and while technically invalid expect them to work without XB crashing.
I'll keep assigned to me. amangrover90 from my team is picking this up and I pinged him to make sure he reviews. I like the approach, good idea @wim leers!
+1 @dpacassi your MR worked perfectly. Closing mine for https://git.drupalcode.org/project/gin/-/merge_requests/510
@dpacassi sorry for the delay, but testing the patch now with rc14. Just rebased the MR
Copying in the full error so people find this issue more easily.
So its override may lead us to lost some features.
That is fair. Right now the Gin theme is completely broken if the module is turned on. What if we leveraged ignore missing to allow progressive enhancement once the toolbar-button.html.twig is added back?
From the docs: https://twig.symfony.com/doc/3.x/tags/include.html
You can mark an include with ignore missing in which case Twig will ignore the statement if the template to be included does not exist. It has to be placed just after the template name. Here some valid examples:
{% include 'sidebar.html' ignore missing only %}
I just started, not on the main team though. I'll check that issue tomorrow and raise it internally.
🫠 apparently I search all the wrong ways. Sorry, I wanted to avoid making a duplicate.
I'm working on XB and using Gin. We're going to need the top bar, so we'll patch around it.
Not sure how heavy to go into the tests.
mglaman → created an issue.
I know one thing. Find all items like assert($entity->hasField('field_xb_demo')); to support pages and their components field.
Assigning to @laurii to help flush this out. The remaining item is 📌 Page should have author/user reference Postponed: needs info but I'm not entirely sure what is next. Maybe pivoting from `node/article` to `xb_page` and pushing forward other features in XB. I'm not sure.
Replied about the enhancer. And the test passed once I added video recording to debug... so let's see if it passes again when I remove videos.
I have to completely rewrite ApiPreviewControllerTest due to its usage of XBTestSetup. Unless we skip defining dependencies in the test base class, which is completely useless. Perhaps it should have all gone into a trait as I was wondering in my MR.
Because you cannot add dependencies to be installed on the base test. The test this issue added runs the site setup install script, which manually installs modules. It assumes a non-kernel test environment.
Thanks
Per #3452712-16: Possibly script loading/placement issue → and the reference core issue, this should fix some problems. We need the main library in the header. But AJAX should be okay without that toggle.
Here's where/why it was added: 🐛 Possibly script loading/placement issue Fixed
There's a reason we needed it to be in the header. Unfortunately I can't recall why. I'll look.
This copied \Drupal\Tests\experience_builder\Kernel\ExperienceBuilderTestBase and broke any test using it when dependencies are installed due to how \Drupal\Tests\experience_builder\TestSite\XBTestSetup operates.
Needs review. I think we can update some @todo to point to
📌
Consider removing baseQuery in favour of explicity requiring the params in each api call
Active
.
#20 still requires the field module to be installed, even though generic field support has been added:
// Call proxy implementations.
if (\Drupal::moduleHandler()->moduleExists('field')) {
_field_token_info_alter($info);
}
The module exists check can be removed.
And then I saw
$provider = '';
if (isset($info['types'][$token_type]['module'])) {
$provider = $info['types'][$token_type]['module'];
}
if (!($field instanceof FieldStorageConfigInterface) && $provider != 'token') {
continue;
}
Adding as feedback to that issue.
mglaman → changed the visibility of the branch 3479454-php-8.3-deprecations to active.
Applied suggestions, thanks @wim leers. I want to clarify how in depth we should provide a suggested solution for point 3 on.
The MR has been updated to use the approach found in the json_field module, where SQLite uses the "text" type, avoiding the core patch.
Okay, so what's left? Add this line to each driver? I don't know what value the in-code comment would provide. Or would the comment be for why we only added it to SQLite and not the others?
// @note: Only the SQLite driver has this field map to due to a fatal error caused by this driver's schema
// @todo: Add to all drivers in XYZ
'json:normal' => 'JSON',
Something like this?
re #15
1. tests can be run with mysql locally, and it's one test that fails on sqlite without the patch
3. blame Drupal core for not providing enough ways to govern things via code and magically through config objects 😬, I'll try to find a way though.
@wim leers metatag uses text for all? https://git.drupalcode.org/project/metatag/-/blob/2.0.x/src/Plugin/Field.... I think I know what you're talking about, though. Is it the json_field? https://git.drupalcode.org/project/json_field/-/blob/8.x-1.x/src/Plugin/...
Note they use text for the sqlite_type! XB does not.
'columns' => [
'value' => [
'type' => 'json',
'pgsql_type' => 'json',
'mysql_type' => 'json',
'sqlite_type' => 'text',
'not null' => FALSE,
],
],
];
re #9 I highly doubt ✨ Add "json" as core data type to Schema and Database API Needs work is going to make enough progress anytime soon.
we should at least at testing for the change and a comment about why the change is done.
I'll see if I can make a quick enough test. Hopefully without a field type and just manipulating a table with a JSON column.
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 → changed the visibility of the branch 3487533-cannot-delete-a to hidden.
ugh picked the wrong branch
Thanks! I appreciate helping keep dependency tree at a minimum
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