🇺🇸United States @tedbow

setting to Needs Review. I think I have addressed the feedback but the pipeline is failing because of 403 errors in UI and Composer jobs. I think it is related to cloudflare issues https://www.cloudflarestatus.com/incidents/gshczn1wxh74

Will try to re-run the job in a bit

@wim leers @larowlan thanks for the reviews, will address

tedbow → created an issue.

I am not sure why yet but when manually test the default values for radio buttons don't show up

Steps:

1. Add 3 fields to article, 1 integer list, using radio buttons, 1 string list using radio buttons, 1 text field

   make sure all 3 fields have default values

2. Add an article, all 3 default value should be set in the form
3. open the article in XB
4. The text field default value is correct but no values are selected for the radio buttons
5. Select values for the radio buttons, and change the value of the text field
6. reload the page
7. The value for the text field retains the updated values, the radio buttons are not selected again
8. Select values for the radio buttons(make sure not to use default), a
9. publish the node
10. The radio button fields are not correct, in case they completely different from the default and what I selected

I am going to test 0.x to see if some of these problems exist

Before we can start doing the back-end changes for this, will to decide on the 1 current item in "Remaining questions"

This is really a product question so assigning to Lauri

My vote would be option 3. We require the client to list of all entity keys regardless of whether they will be published but all have to match the auto-save has for changes that will be published.

This seems like a good balance of making sure the user is aware changes but also not stopping publishing of some items if another item is actively being edited in XB

tedbow → created an issue.

wim leers → credited tedbow → .

I think 📌 Media Library dialogs triggered from page data do not have buttons yet Active caused 🐛 "There are no users matching "/" whe publishing a node Active . Think we need to postpone on that because it changes the entity fields and will always show a change after publish

Looks like 📌 Media Library dialogs triggered from page data do not have buttons yet Active caused this.

Reminder to self, did this also reset the "sticky" property?

I am not sure why but manually testing at previous commits I figured out this issue introduced 🐛 "There are no users matching "/" whe publishing a node Active . It might also be resetting sticky

Some suggestions but think it looks pretty good!

tedbow → created an issue.

whoops, forgot this needs tests

based on @larowlan approval

Or, to put it another way, if you're updating a package you already have, minimum-stability should be ignored. It should only matter if you're adding something you don't already have.

I don't think this exactly true if you minimum-stability: beta you have an existing page that is at 1.0.0-beta1 you should not be able to update to 2.0.0-alpha1 because even though it is an update it is going from below at minimum-stability to below it

Obviously we need tests because nothing is failing now because of this. Also should be stable blocker

I wonder if we want a more complex e2e test where we say start with no media item select load a media item and then remove it again. This should return to the start state and not show a change but I wonder if it actually would

tedbow → created an issue.

I just noticed the comments at the end of \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer are wrong

// Update the entity, validate and save.
    // Note: constructing ComponentTreeStructure from `layout` and
    // ComponentInputs from `model` also included validation. But that
    // included only structural validation, not semantical validation.
    // @see \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem
    // @see \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator
    // @see \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraintValidator
    return [
      'tree' => json_encode($tree, JSON_UNESCAPED_UNICODE | JSON_FORCE_OBJECT | JSON_THROW_ON_ERROR),
      'inputs' => json_encode($inputs, JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR | JSON_FORCE_OBJECT),
    ];

re "Update the entity, validate and save." we don't do any of this here any more, probably did from where ever this was refactored from .

Don't know if I should make a new issue or fix or this is going away in the current issue

probably needs test to prove we now have the entity context. this should result in the client receiving entity context when there are errors

tedbow → created an issue.

We still have to do point to this issue. created MR to remove this

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

@larowlan re #2 about this being a stable blocker

I agree we should not data loss if 2 users are editing the same entity but I wonder if our goal should be to support this workflow in 1.0 or just prevent it from happening. Just technically supporting doesn't seem like enough, we should have good UX around it and if we can't achieve that it seems like it would better to not support it all.

updated the summary based on @lauriii comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

I assumed a single permission for both asset libraries and code components but asked for clarification here https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

Going to take a break from this right now, so unassigning myself.

There are 2 different MR's

1. 795 changes to handle a component not having if an input for a component if \Drupal\experience_builder\ComponentSource\ComponentSourceInterface::requiresExplicitInput returns for false for the component
2. 759 just makes sure \Drupal\experience_builder\Controller\ApiLayoutController::buildLayout() sets an empty array in the model if \Drupal\experience_builder\ComponentSource\ComponentSourceInterface::requiresExplicitInput returns for false for the component

795 personally feels more correct for me.

None of the test failed with "No props sources stored..." error I mentioned in the comments. Probably this means we don't test this situation. It don't think it happens if you try to publish an XB field that you just added a no-props component. It only happens when it was only published once with a no-props component and then you publish again.

Will try to update \Drupal\Tests\experience_builder\Kernel\ApiAutoSaveControllerTest::testApiAutoSaveControllerPost with a test case for this

@roshni upadhyay thanks for the starting this. Needs work for MR comments and phpstan test failures

@roshni upadhyay , just a note that when you set to Needs Review make sure to unassign yourself so others know that somebody else has take on the review

Is this something that is still be considered? If so it would probably be done before stable, correct?

I think needs to be stable blocker or we need to figure out a different solution that would allow FieldTypeUninstallValidator to work with all supported databases. Otherwise I think nothing would prevent you from uninstalling this module while the field type is still in use

It seems like we should at the very least document, possibly with a in UI warning if you are using Safari before stable

It seems like this should be stable blocker decide at least if we want to do this. If we do it is probably better before stable otherwise I think we would have to write an update hook that would update all content entities to use the new structure.

I think when you look at the `CONTRIBUTING.md` as a whole after this change it is bit confusing.

Top level title is "See it in action + recommended development environment"

then 1st heading "Standard installation"
Then on the same level "Using docksal"
Then again on the same level "During development", where we mention https://github.com/TravisCarden/ddev-drupal-xb-dev and cite commands that it provides.

Given we have "recommended development environment" in the main title it is not clear what we are actually recommending

re

But in principle, the same problem is true for JavaScriptComponent and AssetLibrary config entities — it's just far less prominent.

I think this is actually pretty easy to make happen. As soon as your have existing published component all you have to do is click "edit" for the component and this happens. So there is no way to view the code of currently published component without also having it show up in "review x changes"

Currently it seems like deleting a page is pretty basic functionality

This will affect any page with a no-props component and it is not clear to the user what the problem is

Seems like this needs to be fixed by stable

It seems like this should be a stable blocker, at least to decide if we are going to use Workspaces or Workspaces Extra for 1.0.0. The other option would be to continue to use our current auto-save -> Publish all functionality.

It may be more practical to use the current functionality if we are willing accept some of the limitations.

I did some further manual testing.

it seems that with the current MR and the standard profile the parts of entity_form_fields that still change are field_tags and field_image, both entity reference fields.

So I

1. Installed the standard profile and enabled xb_dev_standard
2. deleted both the tags and images field from Article
3. created an article
4. opened it in XB
5. changed the title
6. published the change
7. waited for 10 seconds to see if "review 1 change" showed

In the MR there doesn't result in "review 1 change" but 0.x does

In 0.x the difference between the call from AutoSaveManager::recordInitialClientSideRepresentation and AutoSaveManager::save is in save() there are 2 extra items under entity_form_fields ,advanced__active_tab and revision

So far all I have done in the merge request is bring over my attempts in 🐛 Auto save shows a change when there is not one Active to try to make entity_form_fields not show a difference after entity is published and there no additional changes. That was before I split that problem in to this issue

The current changes do reduced number of changes in entity_form_fields but didn't eliminate them

@wim leers re #26

But the consequence is that this MR will have to do some cache context trickery.

I don't actually need to calls base_path() so hopefully this issue won't have extra cache problems. see https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

also I added a todo to 📌 Define xb_path entity key for identifying entity path field Active to figure out the 'path' key dynamically

Let us never speak of #19 again

🤦🏼

I think I posted #19 on the wrong issue. I don't how I remembered which issue it was supposed to be on but I did. I see I made a very similar comment here #3471028-30: [PP-1] Expand test coverage in FieldUninstallValidatorTest → and on the same day. There it actually makes sense. So must of posted it here first by mistake and forgot that I made it here.

I blame the abundance of browser tabs in our modern world😜

It is also unclear why we are doing this issue . AFIACT from the discussion in #3500052-27: Provide HTTP API for listing Page content entities that can be updated by the current user → it is so in the navigation and the site is sub-directory we should /page/1 or /page-1-alias and not /subdir/page/1 or /subdir/page-1-alias. But it wasn't actually stated directly

If that is case we should put that in "User interface changes"

tedbow → created an issue.

@mayur-sose sorry. The instructions for reproducing the issue have changed since the issue started see #17. The current summary has the new instructions

The title problem was split off into 🐛 Changing a value in Page form then publishing, Auto-save shows a change when there is not one Active which still is not fixed.

Can you test this with the updated instructions in the summary as they are now? Thanks.

Also re

6) Wait for few second and click anywhere on the page

I think the client only calls back the server every 10 secs to check for new changes, so you should wait 11 to be safe(though the change might show up soon depending on when it last checked)

I am not sure that clicking anywhere on the page is necessary.

The wait could have been updated but I am not aware that it has

updated title and summary for alias updates

I chatted with @jessebaker about this.

This is about updating the path in the navigation to match the alias field if it was changed by the user

I am working on this

@traviscarden nope it has tests now

My guess is that this new problem I found is related 🐛 Changing a value in Page form then publishing, Auto-save shows a change when there is not one Active
because small difference in entity_form_fields result in a change being shown in "Review X changes" when there isn't one.

This seems like a similar problem space to 📌 Media Library dialogs triggered from page data do not have buttons yet Active because that is changing on the way entity_form_fields is set

create 🐛 Changing a value in Page form then publishing, Auto-save shows a change when there is not one Active for 2nd problem mentioned in #17

tedbow → created an issue.

I have updated the steps to reproduce because I think the original problem found on [#16007273]
- https://www.drupal.org/comment/16007273#comment-16007273 →
did involve changing the the title

I think it was caused by the different ordering of keys under components.

I think there 2 things going on that can cause the "Review 1 change" problem after you publish

1. the keys under a component are in a different order
2. If you interact the Page Data form, then values in entity_form_fields, are not exactly the same

I think the sorting I added solves 1). I started to work on 2) and got some ways in making entity_form_fields but not completely. I think that is going to be tricky to solve so I am going to open up another issue for that. I reverted my tries to fix it.

working on the js test

Assigning to @larowlan as he is the only one that covers all the CODEOWNER areas

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

This lead me to discover 🐛 Trying to repulish a node that has an existing no props component does not work Active . I think we should just commit this issue with the key sorting for now and the test coverage for that. Then see what the larger problem is for components without props

tedbow → created an issue. See original summary → .

back to this one 🔨

re #5
I did a quick search of `core/modules/workspaces/tests` for the work "transaction" and didn't find any results

I tried multiple ways to try to get this happen, and I couldn't

I can't reproduce this from the directions in the summary.

the only thing I couldn't do exactly from the instructions is, after I clicked "add to components" it automatically closes the editor, so I had to reopen.

Looking into this

going to work on another issue

This is handled now as of 🐛 Deleting a content entity should clear its values from the auto-save store Active

@wim leers I added another component to the existing \Drupal\Tests\experience_builder\Kernel\ClientServerConversionTraitTest::testConvertClientToServer

But also noticed this doesn't have tests for slots that filled so I will add that here too.

Manually testing the MR now I can get to not lose the default image when changing the heading property.

My solution was to set the props that were missing from the client in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput

Not knowing this part of the code it seems reasonable to allow the client the skip props that don't have a value, since the server can figure out this info.

Will see if this breaks other tests

I am wondering if this is front-end issue. I am not super familiar with how the front-end sends back the form inputs but this is what I am seeing using the optional-props-image‎ component I added in the MR

1. Add the component to the canvas
2. open the edit form
3. start to type in the heading
4. the image goes away in the canvas

Between step 3 and 4, I debugged the call to \Drupal\experience_builder\Controller\ApiLayoutController::patch()

The body of the request has model->source->heading and model->resolved->heading but 'image' is not under either shouldn't it be sent back under 'source' even though it is empty?

Looking at the value in the client form of edit-form-xb-props I see

{
  "name": "XB test SDC with optional text and image",
  "resolved": {
    "heading": {
      "value": "Heading the right direction?"
    }
  },
  "source": {
    "heading": {
      "expression": "ℹ︎string␟value",
      "sourceType": "static:field_item:string",
      "value": {
        "value": "Heading the right direction?"
      }
    },
    "image": {
      "required": false,
      "jsonSchema": {
        "title": "image",
        "type": "object",
        "required": ["src"],
        "properties": {
          "src": {
            "title": "Image URL",
            "type": "string",
            "format": "uri-reference",
            "pattern": "^(/|https?://)?.*\\.(png|gif|jpg|jpeg|webp)(\\?.*)?(#.*)?$"
          },
          "alt": {
            "title": "Alternative text",
            "type": "string"
          },
          "width": {
            "title": "Image width",
            "type": "integer"
          },
          "height": {
            "title": "Image height",
            "type": "integer"
          }
        }
      },
      "sourceType": "static:field_item:entity_reference",
      "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟entity␜␜entity:file␝uri␞␟url,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}",
      "sourceTypeSettings": {
        "storage": {
          "target_type": "media"
        },
        "instance": {
          "handler": "default:media",
          "handler_settings": {
            "target_bundles": {
              "image": "image"
            }
          }
        }
      }
    }
  }
}

So it does have the info for image. should it be sending back what it has?

Chatted with @longwave. I am going to take a look at this since he is on another issue