🇺🇸United States @tedbow

Looks good!

Good if tests pass!

1. if Package Manager is installed at the command line (as is the case in the Starshot prototype, or any time Drush installs Package Manager).

   Why does this apply more to command line then the web install? Won't the logic be the same in either case?

2. What happens if you installed package manager locally, the config is automatically set to /local-path/composer but then you deploy the site to server with config and composer is at /server-path/composer. Won't package manager fail because composer is not at /local-path/composer and won't try to find it because the config is set.

   As it is now if you install package manager locally and package manager can find Composer and then you deploy to a server and Package Manager can also find Composer but it is at different path then it will work in both places.

   But as I understand this change depending on how you deploy config then this will no longer work.

3. If 2) is right could you not just handle this in the Starshot repo as it more defined use case where you can assume more things.

   Especially since I don't think we have ever had a issue filed from actual use complaining about this problem. My guess is usually Package Manager finds Composer and Rsync without a problem

@Wim Leers I changed this to use symfony/validator constraints. I think this is better.

I still need to add some test cases for missing keys and type errors but this logic should already be covered by the constraints.

So I have still have work on it. If you want to just wait until I am done, I can just continue working on it. But if you want to look at the approach feel free

Wim Leers → credited tedbow → .

tedbow → created an issue.

I would hope we could still do the core alpha version without this. Not that is not a good idea, but there a lot of good ideas and I don't think we can block on all of them. Also if this were a core issue after it were part of core would have many more eyes

Not saying we should postpone it though, just in the limited time I currently have for AutoUpdates I will probably be focusing more on The Update Framework

@catch something like this seems like a good idea.

Some questions,

If a release is a minor update, and the version number ends with .0 don't show it by default, or show it with a warning.

Does it matter how old the .0 is? Or should assume there will always be a .1 soonish because there will always be something to fix?
Like what if .0 is 1 month old but also it is the latest in the branch?

We know both from our own sites that there is non-zero work updating to a minor release, even if it's just testing.

Regarding the .0 warning is this not the same or almost the same if you were going from say 12.0.7 to 12.1.1 if you happen to miss the 12.1.0 because you use the form and not the background updates?

Related

Generally we find these issues during rc and the first month or so after a .0 release, and between core patch releases and contrib updates they usually settle down after a month or two.

so maybe instead of warning based on .0 should it be basedon minor updates to the site within 1 or 2 months after that minor has been released?
Like it is updating 12.0.7 to 12.1.4 but 12.1.0 just came out a week ago should still have a warning?

tedbow → created an issue.

Think this is ready for review. I used the core/php.xml.dist file before 📌 Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard Fixed was committed.

You can see the shows a new warning that our core.phpcs.xml.dist file is out of date but phpcs job passes. https://git.drupalcode.org/project/experience_builder/-/pipelines/231385

I also ran pipeline with were I copied the most recent core/php.xml.dist. This passed the new job to check to see if the phpcs was up to date but failed the phcs job because we are not up to date with all the rules. https://git.drupalcode.org/project/experience_builder/-/pipelines/231381

Looks good

I specifically opted to chase it because otherwise we get the feedback/complaint that we’re trailing HEAD too far.

Are we going to be in core anytime soon? I am unsure why it matters if we are month or 2 behind. does this really change that much?

Since I doubt anything will get into core before the demo in Barcelona it seems that we should favor development speed over keeping up with core's phpcs rules.

then? That'd match Drupal 10.x's rules, which won't change as much.

That seems better but 📌 Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard Fixed was still committed to 10.x branches.

I am going to change the MR to a draft try a couple of ideas. Basically allow us to get a warning if we haven't updated in phpcs rules in X number of days

tedbow → created an issue.

re #19

The correction in #18 I was referring to was actually for #14.1

not #14.2

@Wim Leers sorry for further confusion

but originally in #14 I typed

So I think this tree would valid but it does pass your suggested validation

But meant this.

So I think this tree would valid but it does NOT pass your suggested validation

You responded to this in #17 so wanted to called that out.

I edited #14

@Wim Leers assigning back to you for questions on #14. If you have pointers for #15 that would be great(otherwise when I get back this issue I will keep looking)

See MR comment

@Wim Leers I am working on this part now

slots of every component: the stored slot names (firstColumn and secondColumn in the example at 📌 FieldType: Support storing component *trees* instead of *lists* Fixed ) MUST be actually existing slot names for this particular component instance.

I haven't actually dealt with this part of the module at all. Any tips here. How do I load a component from an id? Is this always config component as defined by \Drupal\experience_builder\Entity\Component? there is no class comment so hard to tell. Or could this any sdc component. I have loaded a Component config entity but also there I can't see slot info there.

re #12

1. 1. What if there is a tree of 3 levels deep, and the first level is valid (i.e. all UUIDs under the root also have top-level keys), and the third level is valid, but the second is not? (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

   and

   👆 these are edge cases that I described succinctly in the issue summary as any top-level UUID appears as a UUID in a _parent branch/tree_(except its own branch)

   I don't think this first case is actually what is described in the summary but rather the inverse(unless we are still not talking about the same of constructing the tree from the storage)

   (i.e. all UUIDs under the root also have top-level keys)

   UUIDs under the root should not need to have top-level keys, some will but components without slots should not need any other information in the tree. I confirmed with Jesse Baker that Components are not required to have slots.

   (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

   There is a typo here I think but again I think a child of the first level should not be required to exist as top level key because the child of the first level might not have a slot that needs representing in the tree.

   So I think this tree would valid but it does pass your suggested validation

   [
     "ROOT_UUID" => [
         [ "uuid" => "UUID-IN-ROOT-NOT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
         [ "uuid" => "UUID-IN-ROOT-AT-TOP-LEVEL" , "component" => "component-with-slots" ] , 
     ],
     "UUID-IN-ROOT-AT-TOP-LEVEL" => [
       'firstCol' => [
           [ "uuid" => "UUID-UNDER-1ST-LEVEL-NOT-AT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
           [ "uuid" => "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL" , "component" => "component-with-slots" ] , 
       ]
        
     ],
     "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL"  => [
         'firstCol' => [
              [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
              [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
        ]
       
     ]
   ]
   

   Sorry for the weird naming basically I thought "component-with-no-slots" will be dead-end in the tree and that is by design.

   I have updated the test cases to use either "component" => "component-no-slots" or "component" => "component-slots" to make this clearer.

2. "dangling" aka leftover top-level keys, for components that have been removed. These would effectively be dead weight that continues to exist, taking up storage space while not being visible!

   I think that is what this existing case is catching

   'INVALID: valid tree, with unknown top level' => [
           [
             ComponentTreeStructure::ROOT_UUID => [["uuid" => "uuid-in-root-only", "component" => "component-no-slots"]],
             "uuid-unknown-top-level" => [["uuid" => "uuid-under-top-level", "component" => "component-no-slots"]],
           ],
           ['The UUID <em class="placeholder">uuid-unknown-top-level</em> is not in the tree.'],
         ],
   

   but I have expanded the tree here
   'INVALID: valid tree, with unknown top level' => [
   [
   ComponentTreeStructure::ROOT_UUID => [
   ["uuid" => "uuid-in-root-only", "component" => "component-no-slots"],
   ["uuid" => "uuid-in-root-and-top-level", "component" => "component-slots"],
   ],
   "uuid-in-root-and-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-1st-tier-top-level", "component" => "component-slots"],
   ],
   ],
   "uuid-1st-tier-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-under-1st-tier", "component" => "component-no-slots"],
   ],
   ],
   "uuid-unknown-top-level" => [
   "slot1" => [
   ["uuid" => "uuid-under-top-level", "component" => "component-no-slots"],
   ],
   ],
   ],
   ['The UUID uuid-unknown-top-level is not in the tree.'],
   ],

3. I fixed some of the test cases that were not nested correctly.
4. RE: Do we want to add validation that if component has slots it should have top level key? My guess is not because that would require use to load the actual component config and also maybe some have optional slots but just putting this out there we don't validate this.

5. Can you add a failing test case to \Drupal\Tests\experience_builder\Kernel\Config\DefaultFieldValueTest that triggers a failure for this new constraint for a config-defined component tree?

   Yes, will do this, but not done yet.

   Should we also add the test cases to ComponentTreeItemTest::testInvalidField() to make it sure it triggers for field instances? Right now they use the same helper function, ComponentTreeTestTrait::getComponentTreeTestCases() to provide test cases so we test the same cases in both places

6. I think the test cases would be much simpler to read if they weren't defined as JSON strings, but as json_encode([ … stuff … ]) instead.

   Agreed, fixed

Leaving assigned to myself to do the remaining items but I need confirmation from @Wim Leers that I am right about 1)

tedbow → created an issue.

re #19 Created the follow-up 📌 Create a new Exception type for a dynamic prop missing a host entity Active and fixed 📌 CI: phpcs job failing due to upstream changes in Drupal core: unused variables in catch statements now disallowed Fixed

tedbow → created an issue.

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

Did #17, had to make new test trait for this. Also made some change to `ComponentTreeItemTest` because in refactoring to the new test class I realized setUp() and the class constants where needed for testCalculateDependencies() which I think was confusing and made changes that were not needed for the new test method to pass. See MR comments

Agree with #27 this is outdated

fixed the other file, can revert if needed

Fixed the phpcs problem expect for 1 in themes/engines/semi_coupled/semi_coupled.engine which was not changed. I guess there is new rule in core?
Anyways I could fix this other file too if we want to get green

fixing phpcs

RE

The piece still missing from this MR: explicit test coverage for type: type: field.value.component_tree — i.e. when there is a component tree defined in configuration.

Isn't this covered in

\Drupal\Tests\experience_builder\Kernel\Plugin\Field\FieldType\ComponentTreeItemTest::testDefaultFieldValue

where it expects the error Schema errors for field.field.node.article.field_xb_test with the following errors: 0 [default_value.0] The array must contain a "tree" key.. This almost the same error you pointed out just for different field. This is save configuration or did you mean explicitly in config .yml file?

Added more test cases. I think this is ready for review.

I have 1 question/comment on the MR but I think that can be handled in follow-up, if needed

@Wim Leers I am still working on this(but finished for the day) but feel to take a look to see if totally on the wrong track anywhere.

The pipeline https://git.drupalcode.org/project/experience_builder/-/pipelines/222097 says it is "blocked" but I am not sure what is blocked on

I ran all the jobs and it passed

Postponing on 📌 FieldType: Support storing component *trees* instead of *lists* Fixed

tedbow → created an issue.

Pipeline is passing

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

Marking this as Critical as blocks 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed which is critical

We should fix 🐛 All field item constraints are not found when evaluating prop matchs Active first

tedbow → created an issue.

tedbow → created an issue.

Looking into this

Putting this down to work on 📌 Refactor and break up SdcPropToFieldTypePropMatcher Needs work

@larowlan on 2nd thought I am going to unassign this because I think your feedback was pretty straight forward. Welcome your review though

@larowlan, thanks for the review. Addressed your feedback

Will give this a try

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

I am taking a look at this

I think I have found a solution in 📌 Allow Experience Builder fields to support Asymmetric and Symmetric translations Needs review . If that solution does not work we can re-open this

Working on this

I think maybe we can close #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity → as I think the use of hook_entity_prepare_view is an okay solution here. But I will research #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity → a little more to be sure.

but I think hook_entity_prepare_view is a least a good solution for now and we could leave #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity → open if we want to ask for some core change

I figure out the problem with viewing entity as a translation and having the XB field being untranslated and showing the title of the default translation.

I figured out why this happens.

1. \Drupal\Core\Entity\ContentEntityBase::get() calls \Drupal\Core\Entity\ContentEntityBase::getTranslatedField
2. getTranslatedField has

   if (!$default && !$definition->isTranslatable()) {
           if (!isset($this->fields[$name][LanguageInterface::LANGCODE_DEFAULT])) {
             $this->fields[$name][LanguageInterface::LANGCODE_DEFAULT] = $this->getTranslatedField($name, LanguageInterface::LANGCODE_DEFAULT);
           }
           $this->fields[$name][$langcode] = &$this->fields[$name][LanguageInterface::LANGCODE_DEFAULT];
         }
         else {
   

   So in the case the XB field is not translated it do a recursive call to getTranslatedField() but his time using LanguageInterface::LANGCODE_DEFAULT

3. In the recursive call to getTranslatedField() then this is called
   

   $field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this->getTranslation($langcode), $name, $value);
   

   So $this->getTranslation($langcode) means the field item list will be created with the default, LanguageInterface::LANGCODE_DEFAULT translation, even though the entity that had the original call to \Drupal\Core\Entity\ContentEntityBase::get() in step above could have been the translation of another language.

The change was made in #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity →

$field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this, $name, $value);

was changed to

$field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this->getTranslation($langcode), $name, $value);

I will read up that issue. But presumably there was a good reason to need the translation instead of the original entity object.

I was able to use hook_entity_prepare_view to set the fields entity parent to the correct translation

Ok, this change was made in #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity →

$field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this, $name, $value);

was changed to

$field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this->getTranslation($langcode), $name, $value);

I will read up that issue. But presumably there was a good reason to need the translation instead of the original entity object.

Ok I think figure out why this happens.

1. \Drupal\Core\Entity\ContentEntityBase::get() calls \Drupal\Core\Entity\ContentEntityBase::getTranslatedField
2. getTranslatedField has

   if (!$default && !$definition->isTranslatable()) {
           if (!isset($this->fields[$name][LanguageInterface::LANGCODE_DEFAULT])) {
             $this->fields[$name][LanguageInterface::LANGCODE_DEFAULT] = $this->getTranslatedField($name, LanguageInterface::LANGCODE_DEFAULT);
           }
           $this->fields[$name][$langcode] = &$this->fields[$name][LanguageInterface::LANGCODE_DEFAULT];
         }
         else {
   

   So in the case the XB field is not translated it do a recursive call to getTranslatedField() but his time using LanguageInterface::LANGCODE_DEFAULT

3. In the recursive call to getTranslatedField() then this is called
   

   $field = \Drupal::service('plugin.manager.field.field_type')->createFieldItemList($this->getTranslation($langcode), $name, $value);
   

   So $this->getTranslation($langcode) means the field item list will be created with the default, LanguageInterface::LANGCODE_DEFAULT translation, even though the entity that had the original call to \Drupal\Core\Entity\ContentEntityBase::get() in step above could have been the translation of another language.

I am not sure why this happens but it seems very intentional. Will investigate further.

All passing

Created follow-up for 🐛 If Experience Builder field is not translatable the dynamic source properties do not use the translated entity field value Active

tedbow → created an issue.

Wim Leers → credited tedbow → .

tedbow → created an issue.

tedbow → created an issue.

Wim Leers → credited tedbow → .

add child issue Assigned to: tedbow 📌 Allow Experience Builder fields to support Asymmetric and Symmetric translations Needs review

tedbow → created an issue.

@Wim Leers I would prefer not to postpone this issue. I think we should still explore if the DB's support what we need. Regardless of what nice API core puts around the DB differences we will need the DB's to support the basic functionality or be ok with the work arounds we would have to implement.

I think it would be also good to start to list the ways we think we need will need to search the json field and whether those needs will involve this unknown parent keys problems.

An example

1. Entity queries. An an example $query->jsonPropertyCondition($xb_field, 'props', $json_path, '%some string%', 'LIKE')

   Outside of the use case of 📌 [PP-1] Prevent modules from being uninstalled if they provide field types used in an Experience Builder field Postponed and other logic we might need in uninstalls, where maybe we could make utility functions, we would need to search without knowing all the parent keys?

   From our previous test data
   'props' => '{"dynamic-static-card2df":{"text":{"sourceType":"dynamic","expression":"\u2139\ufe0e\u241centity:node:article\u241dtitle\u241e\u241fvalue"},"href":{"sourceType":"static:field_item:link","value":{"uri":"https:\/\/drupal.org","title":null,"options":[]},"expression":"ℹ︎link␟uri"}}}',

   If we wanted to be able to do this we would to have unknown parent keys, because if we wanted to search for any link field in any component that linked to the domain drupal.org we would never know the component IDs so that would be at least be one parent key we would not know.

But do we for need this kind of ability?

I am going to try experiment to get the \Drupal\experience_builder\FieldTypeUninstallValidator::checkContentEntityUses to work with all DB's by making a utility class

@Wim Leers I have started to look at ✨ Add "json" as core data type to Schema and Database API Needs work and made a couple comments as I start to understand it. It doesn't seem to solve this problem yet.

I asked in @bradjones1 about this in Drupal slack to see if he has any ideas or run into this problem.

Hi, I actually just found out about this module, or maybe I forgot about it

I am one of the maintainers of the Automatic Update module and tech lead of the initiative to get it into core. We have now had a stable release for a while. The main module just updates Drupal core(because that is the core MVP) but we now also have a sub-module that updates contrib modules and themes

Some thoughts on:
#7

Let's postpone this issue until a new UI (which we understand Automatic Updates will eventually be) will be around the corner.

and #11

Once the fully functional Automatic Updates module will become the part of Drupal Core - the Ludwig module will retire.

Getting into core is very, very slow process 😢

One of the main blockers is getting the The Update Framework implemented on Drupal.org. But this is above and beyond what virtually any site using Composer has now and I assume the level of security Ludwig itself has.

So as a replacement for Ludwig I think the contrib Automatic Updates is ready. The MVP version of Core Automatic Updates won't have contrib updates anyways.

Automatic Updates does not support installing new modules but Project Browser → does and it leverages Package Manager which is a sub-module of Automatic Updates(will be its own module in core)

Package Manager is really a Composer API module that can be used other composer operations.

Another module that might be of interest is Automatic Updates Extras → which I created as place for update/composer related things that are not likely for the Drupal core path. If there are features that the main Automatic Updates and Project Browser don't have but are needed by users of Ludwig that could be place for them.

For example we could add a "convert to composer" feature to Automatic Updates Extras. Or you could add that to Ludwig itself using the Package Manager API.

Anyways let me know your thoughts or you have questions about the state of the Automatic Updates initiative. I assume there might be a lot of overlap between Ludwig users and the intended users of Automatic Updates/Project Browser.

If you want to you could be link to these projects on our project page under Switch to Composer! letting users know these provide a UI so for the most part they won't have to deal with Composer directly. Right now it is only linked under Ludwig EOL? with a mention of it getting into core

@Gábor Hojtsy it is not current but this is the only one. I need to update it

@ressa good idea! Maybe could put this is a general aue_drush sub-module. I am not sure what other integrations we might have in the future but I think it would make sense to just have 1 drush integration sub-module

@phenaproxima Thanks!

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

@catch @larowlan sorry we really to document better how this works

for now I have created 🐛 Prevent fields from being deleted if they are used in Experience Builder fields Active because the "props" in XB field could dynamically reference an actual field of a entity being displayed. In which case we would want to prevent the field on the bundle from being deleted

This current issue deals with a static field where it basically just uses the "shape" of a field type and stores the values in the props too(what we are calling "static"). In this case the values for the field props would be stored in the "props" column of the XB field