- 🇺🇸United States tedbow Ithaca, NY, USA
updated more.... but still more to update
leaving assigned to myself
- 🇺🇸United States tedbow Ithaca, NY, USA
re-rolled, added some more code comments and updating the summary but not finished. Leaving assigned to myself as I am working on the summary
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
How do I begin reviewing this MR? There's lots of comments here, and the issue summary doesn't talk about the
client_id
andclientInstanceId
this MR is adding.Can we get an up-to-date issue summary to facilitate a review? And ideally also sprinkle
ℹ️ …
comments over the MR to make it clearer still 🙏P.S.: if 📌 Document the reasons for not using Workspaces for saving in 0.2.0 Active had landed, then I'd have asked to update that ADR. In fact … any reason not to do that now, @tedbow? 😇🙏 That'd be even better than an issue summary update!
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 For selective reverting add DELETE auto-save endpoint Active is in.
- 🇫🇮Finland lauriii Finland
Any SDC that has enum without the necessary meta:enum will no longer be available to XB users, and will now appear in the list of disabled XB components at /admin/appearance/component/status, with an explanation of why.
I'm not entirely convinced that we should not allow SDCs without meta:enum to be used in XB because it is possible to achieve an acceptable UX without providing meta:enum, albeit not in all scenarios. For example, for margin-top, you could choose to provide options '4px', '6px', and '8px'. In this case the same value could apply both as the human readable and machine readable value.
We could still allow translating the enum options but keep the machine readable names consistent across translations or we could simply prevent translating these enums.
I don't think this needs to block this issue from being committed. It seems that we could loosen this requirement in future if this would require a non-trivial amount of work to change.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I think 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active will resolve this, postponing
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@tedbow: can you please confirm ✨ Publish all changes within a database transaction to allow rollbacks Active is still okay as a rather than a ?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Closed 🐛 Trying to remove code component from page immediately after attempting to edit it gives 404 error Active as a duplicate of this.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Thanks for pushing this forward!
Unsure how component versions are stored in the config entities - need guidance on how to extract/compare versions!
See
\Drupal\experience_builder\Entity\VersionedConfigEntityInterface::getVersions()
and\Drupal\experience_builder\Entity\VersionedConfigEntityInterface::getVersionSpecificDependencies()
. - 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
All tests passing 🎉
Something that doesn't have e2e tests is the code editor, but we need to fix it to at least send the same values in enum as meta:enum.
Bálint volunteered to write this.@balintbrews For now I'd expect to send the same added values as labels. The only thing to take into account is replacing dots with underscores. Something like:
const getMetaEnumValue = (x: string) => x.replace('.', '_'); const enumValues = [ '3.14', 'b', 'c', ]; const metaEnums = Object.fromEntries(new Map(enumValues.map(value => [getMetaEnumValue(value), value]))) console.log(metaEnums); //{ // "3_14": "3.14", // "b": "b", // "c": "c" //}
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This needs work for:
- multiple remarks on the MR
- but most importantly because it's not yet resolving an author-provided URL to the corresponding oEmbed URL, which is why remote videos refuse to render
Note: I'd be totally fine with splitting this across multiple MRs, so you could already land the "local video" MR immediately.
Finally: this is not yet handling document links.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Making @f.mazeikis' observation easier to find: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Found the culprit. oEmbed videos are passed as-is:
<iframe src="https://www.youtube.com/watch?v=v747p7xEcgg" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>
… which is incorrect.
This should actually render something like
<iframe src="https://example.com/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g" width="" height="" title="" data-xb-uuid="a9a2498e-e1a1-4464-b2ea-db72daf7ea60"></iframe>
Why? Because that's how core renders remote videos successfully. See for yourself by doing:
- Go to
/admin/config/media/media-settings
and enable stand-alone media URLs - Create a video media entity for some YouTube video
- Navigate to
/media/N
- Video appears!
- Put a breakpoint in
OEmbedFormatter::viewElements()
, observe the different steps, specifically how$url
and$resource_url
exist. This results in:
array ( '#type' => 'html_tag', '#tag' => 'iframe', '#attributes' => array ( 'src' => 'http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g', 'scrolling' => false, 'width' => 200, 'height' => 113, 'class' => array ( 0 => 'media-oembed-content', ), 'loading' => 'lazy', ), '#attached' => array ( 'library' => array ( 0 => 'media/oembed.formatter', ), ), )
I think this MR needs to perform that
https://www.youtube.com/watch?v=v747p7xEcgg
👇http://core.test/media/oembed?url=https%3A//www.youtube.com/watch%3Fv%3Dv747p7xEcgg&max_width=0&max_height=0&hash=2kCbWsOQ7urG-UZLzC9KUBuT1B-7jnlX66TsoG_WL7g
transformation, by adding a new computed property on the field definitions for the oEmbed-using
MediaType
bundles of theMedia
entity type. That can be done usinghook_entity_bundle_field_info_alter()
. The MR already needs that for something else too: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...(The
hash
is coming from\Drupal\media\IFrameUrlHelper::getHash()
, the rest from\Drupal\media\OEmbed\UrlResolver::getResourceUrl()
.)This would also result in
\hook_oembed_resource_url_alter()
etc firing (see the example implementation: it rewrites all YouTube URLs to the cookieless variant!) - Go to
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
YAYAYAY!
First local video from Drupal's Media Library rendered via a component:
First remote video (about XB of course — Lee's talk on YouTube!) rendered via a component:
… unfortunately some kind of issue with YouTube refusing things, probably CSP-related, we can figure that out next 👍
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
D'oh — no, actually — I misinterpreted!
This is actually similar to 🐛 Enabling XB for managing page template breaks Olivero header Active , and there's nothing we can do about this.
The reason is essentially the same as #3505629-2: [upstream] Olivero's header works only correctly when using the "block" page variant → :
- Drupal core's
BlockPageVariant
renders the contents of block plugins with an extra wrapper: that's whatblock.html.twig
is about, and that's where<div class="block__content">
is coming from - XB is rendering all block plugins as-is, without adding more wrappers.
We could avoid this by doing something like core's
\Drupal\block\BlockViewBuilder::preRender()
, which is the "view builder" for theBlock
config entity type (which is IRRELEVANT/NON-EXISTENT when using XB!) that is responsible for this, see:// Place the $content returned by the block plugin into a 'content' child // element, as a way to allow the plugin to have complete control of its // properties and rendering (for instance, its own #theme) without // conflicting with the properties used above, or alternate ones used by // alternate block rendering approaches in contrib (for instance, Panels). // However, the use of a child element is an implementation detail of this // particular block rendering approach. Semantically, the content returned // by the plugin "is the" block, and in particular, #attributes and // #contextual_links is information about the *entire* block. Therefore, // we must move these properties from $content and merge them into the // top-level element.
- Drupal core's
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I'm not sure I understand why/how yet, because
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::renderComponent()
uses'#theme' => 'block'
Related: 🐛 Placeholders/#lazy_builder is not supported for block component rendering Active + 🐛 Preview is not accurate for the site branding block Active .
Needs debugging to determine the root cause. Thanks for the detailed bug report!
- 🇬🇧United Kingdom f.mazeikis Brighton
Implemented feedback from #10 ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active , updated
simple/video
SDC component and added newsimple/ombed_video
SDC component. This was useful for testing via UI, but not quite sure if we want to keep it.
Added comments to MR, moving to needs review. - 🇮🇳India libbna New Delhi, India
- Added the array $version_ids = [] parameter to the method signature as requested
- Implemented the filtering structure that loads each config entity when $version_ids is provided
- Maintained backward compatibility (empty array works as before)
What's left:
Need to implement the actual version checking logic inside the filter function (currently has placeholder return true;)
Unsure how component versions are stored in the config entities - need guidance on how to extract/compare versions! - 🇮🇳India libbna New Delhi, India
I have implemented the suggested changes. If there are any additional updates or modifications required, please let me know.
- @libbna opened merge request.
- First commit to issue fork.
- 🇺🇸United States effulgentsia
I moved the "beta blocker" tag to just 📌 Create UI for selective publishing of changes Active and tagged the other children as "stable blocker".
- 🇺🇸United States tedbow Ithaca, NY, USA
changing title to make clear we are only handling content entities, node and pages with layouts
We will need this too 📌 For config entities add client support conflict detection via the `autoSaves' key Active
- @jessebaker opened merge request.
- 🇬🇧United Kingdom jessebaker
In the interests of keeping things as simple as possible on the FE, I was hoping to get something like the following
JSON:
{ "drupalSettings": { "xb": { [...] "permissions": [ "read global regions", "update global regions", "delete global regions", "read sections", "update sections", "delete sections", ... ], } } }
Because then in the UI checking if we should display a button or not is as simple as, for example:
JSX:
<> {drupalSettings.xb.permissions.includes('delete sections') && ( <button>Delete section</button> )} </>
- 🇺🇸United States tedbow Ithaca, NY, USA
@jessebaker thanks for testing this. I was testing this with nodes and it was working.
Basically entity have a seperate revision id but if we create a new revision on every save this won't increment. I guess Page is not incrementing which I guess makes sense because we don't have a way to revert to old revisions or view old revisions AFIACT
So I used the "changed" time also in the revision for the client
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇬🇧United Kingdom jessebaker
There are probably unrelated reasons why the client should always call /xb/api/v0/layout/xb_page/1 after publishing.
As of my latest commit the FE now re-requests the page after publishing changes IF the current page is amongst the list of pending changes.
In my testing there is one remaining bug that I can see that is quite tricky to reproduce.
- In a browser tab start from a blank page, no autosaves (everything published)
- Copy the URL and open a new tab so you have two tabs with the same blank page state.
- In Tab 1, add a Heading component and immediately publish all changes
- Switch to Tab 2.
- Preview is now out of date (showing blank still, but should have a Heading on it)
- Add a Hero component
- There should be a conflict warning but there isn't.
From what I can see, the issue is that the autosave hash has autoSaveRevision but it's always
"1"
. Nothing I do ever seems to increment that number (that is actually a string).{ "autoSaveRevision": "1", "hash": "efa8dd3ce4f5f9e1" }
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
Added the different scenarios of permissions that need to be exposed to the client UI.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
As of #19… I can confirm THE BETA PARTS ARE DONE! 🥳🥳🥳
Signaling this is "done for now", i.e. for 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active but NOT for 🌱 Milestone 1.0.0: Production Sites Postponed , by marking this .
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Following up on #15 and #16 (finally).
#15's
/xb/api/v0/config/component
case was indeed a bug, and was fixed since then in 🐛 The component fails to render the library and props forms due to an unhandled runtime exception. Active , as described in #17.#3518201 and #3518203 are indeed "nice-to-haves" for 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active .
So … adopting the same structure as we have for 🌱 [META] Production-ready ComponentSource plugins Active and 🌱 [META] Production-ready data storage Active to signal beta vs stable vs post-stable.
-
wim leers →
committed cb4c5a3d on 0.x
Issue #3530521 by wim leers, bnjmnm: Decouple image shape matching from...
-
wim leers →
committed cb4c5a3d on 0.x
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The MR for 📌 Decouple image shape matching from the `image` MediaType Active has landed — that provides all the examples necessary for developing, testing and debugging this issue's scope.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Tests are green! Follow-up for next steps exists.
Self-RTBC'ing and merging because nobody else knows the area well enough to review it.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Tests are passing, and are proving that it is now possible to target different fields (i.e. bundle-specific fields) of the same field type across multiple bundles of the same entity type.
This effectively makes this behave similarly to how 📌 Pages have images usable for SEO purposes Active handled the picking of the the image to use in Metatags. (And this will automatically start happening on existing sites with this change.)
And … it actually works thanks to @bnjmnm's work on the Media Library Widget:
Future work
Note: the "same field type" requirement I referred to is simply because the props being targeted in each
FieldPropExpression
cannot (yet) vary by bundle; only the field name can. This could be expanded in a follow-up issue, which would allow for e.g. different media sources that all provide similar media: theImage
MediaSource
(powered by an image/file field type for local images), but a "remote image"MediaType
— for example powered by core'sOEmbed
MediaSource
, using the (disabled by default)Flickr
oEmbed provider.Created 📌 Support image shape matching from the `image` MediaSource: support both the `image` + `oembed` MediaSources simultaneously Active for that.
- @wim-leers opened merge request.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The initial hard technical blocker:
new FieldPropExpression(EntityDataDefinition::create('media', 'image'), …)
needs to be able to be
new FieldPropExpression(EntityDataDefinition::create('media', ['image', 'acquia_dam_image']), …)
… but
\Drupal\Core\Entity\TypedData\EntityDataDefinition::create()
does not allow that:public static function create($entity_type_id = NULL, $bundle = NULL) {
Investigation led to discovering an upstream core issue for this: #2169813: Support deriving fields from entity definitions with multiple bundles → . Which … I happen to have contributed to 8 years ago for JSON:API. 😅👴
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Related but different: 📌 Decouple image shape matching from the `image` MediaType Active .
- Issue created by @wim leers
- 🇫🇮Finland lauriii Finland
I agree that #8 🐛 If user with less/different permissions edits a page after a user with more/different permissions data lost could occur Active would be an acceptable way to get us to stable. This could be then improved post-1.0.0.
- 🇬🇧United Kingdom jessebaker
1) I think perhaps we should create/address this in a follow up
2) +1 to using 'changed' or even perhaps 'lastModified' and using a timestamp - I could see that information actually being useful elsewhere in the UI at some point.
3) After publishing I think we already refetch some data so I don't see it being a problem to update the page. - 🇺🇸United States effulgentsia
I haven't looked at the XB codebase related to this deep enough to comment on the code-level implementation suggestions of #8, but I'm +1 to the high level premise if I understand it correctly, which is:
If an auto-saved entry exists, and there is 1 or more fields for which:
- The value is different than what the published entity has, and
- The current user doesn't have edit permission for it
Then the user should not be allowed to make any changes at all on the Page Data tab. I think ideally the Page Data tab would still be visible, but be read-only, with a message at the top saying that it's read-only because there exist unpublished changes to it that were made by someone with different permissions and must be published or reverted by someone with those permissions before it can be editable by others.
Then we can punt any further scope (i.e., allowing edits of the permissible fields and merging those into the auto-save) to a post-beta, and maybe even post-stable, follow-up.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Just discussed this issue for ~20 mins with @jessebaker and @effulgentsia.
We propose that:
- if an auto-save entry exists for an entity with a field that has granular access control (which is not the case by default) and that field has been changed in the auto-save entry compared to the saved entity (this should NOT be blocked on
#2862574: Add ability to track an entity object's dirty fields (and see if it has changed) →
because
\Drupal\experience_builder\ClientDataToEntityConverter::convert()
takes everything in the entity form and upcasts it to the full entity, but … inaccessible fields would not be shown in the entity form, so shouldn't be in the auto-save entry!) - then the
entity_form_fields
blob returned by theexperience_builder.api.layout.get
route from auto-saveshould be able to check whether the auto-save entries are modifying fields the current user cannot access. - Then we tweak the response returned by that controller (kinda similar to how we did in
📌
Omit `PageRegion` representation from `ApiLayoutController::get()`, 403 if sending it in `::patch()` or `::post()`
Active
), but since the content entity is the primary purpose of this very route, we can't omit
entity_form_fields
— but what we can do is something like not returningentity_form_fields
.
Note: the XB component tree can still be edited, so this user can still make meaningful changes! They just won't be able to edit anything under the tab.
(They are guaranteed to have field-leveledit
access to the component tree thanks to\Drupal\experience_builder\Access\ComponentTreeEditAccessCheck
aka the_xb_component_tree_edit_access: TRUE
route requirement added last week in 📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active having verified that first.)
(Which AFAICT means 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active had some oversights 😅)
Implementation outline of this proposal attached as a patch.
- if an auto-save entry exists for an entity with a field that has granular access control (which is not the case by default) and that field has been changed in the auto-save entry compared to the saved entity (this should NOT be blocked on
#2862574: Add ability to track an entity object's dirty fields (and see if it has changed) →
because
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Merged in upstream, let's get this to passing tests so we can land it 🙏
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The intent has been from the start to avoid adding "Drupalism" to SDC's use of JSON Schema.
That's why we started using
/schema.json
and defined$ref: json-schema-definitions://experience_builder.module/image-uri
.However, given that ✨ [PP-1] Make link widget autocomplete work (for uri and uri-reference props) Postponed is proposing to add a variation for
format: uri
namedformat: uri+entity_autocomplete
(which required a core change: 🐛 ComponentValidator ignores the set validator and creates a new one Active )So, an alternative HERE could be that we start using custom
format
s (fortype: string
). Which is exactly what @lauriii expressed he'd strongly prefer over at #3462705-47: [META] Missing features in SDC needed for XB → . (Note that JSON Schema does support custom format attributes: https://json-schema.org/draft/2020-12/json-schema-validation#section-7 + https://json-schema.org/draft/2020-12/json-schema-validation#name-custom...)Or better yet: use
contentMediaType
.So then we'd go from:
"image-uri": { "title": "Image URL", "type": "string", "format": "uri-reference", "pattern": "^(/|https?://)?.*\\.(png|gif|jpg|jpeg|webp)(\\?.*)?(#.*)?$" },
to:
"image-uri": { "title": "Image URL", "type": "string", "format": "uri-reference", "contentMediaType": "image/*" },
… which is not quite exactly how
contentMediaType
is meant to be used (that's really describing the contents of the string itself), but is … close enough? Something along these lines has been proposed asresourceMediaType
.Examples of actually defining new JSON Schema formats seem impossible to find.
This would then also allow us to apply a similar improvement to "video URLs", which is being added at ✨ Define JSON Schema $refs for linking/embedding videos and linking documents Active .
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#9:
<video poster>
is not semantically the same as\Drupal\media\Entity\Media::loadThumbnail()
.See https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/poster and contrast with
core/modules/image/config/install/image.style.thumbnail.yml
.The media thumbnail is a (typically square) image used when browsing the media library. You cannot use this for
<video poster
: the aspect ratio would be wrong, and the quality is very likely far too low.Conclusion: without the ability to extract an image from a video (we definitely can't do that — Drupal's media system doesn't even do this), we must make do without a
poster
. Which is fine, because as you can see:poster
is optional!just do not populate
poster
by default 👍- Same thing here — they're both optional. Since core's "Video" media type doesn't support it, I don't see how XB could support it.
⚠️ ⚠️ ⚠️ OTOH — what you're dealing with here is that
<video width>
refers to the video display area, not the video's resolution.I bet that this is something @lauriii would like to see supported — even if you could enter bad values (e.g. specifying width/height not respecting aspect ratio etc.), because it impacts the content authoring experience a lot.
However, given the current reality of XB only allowing to use
StaticPropSource
s to populate SDC/code component props, this technically requires a field type + widget that providessrc
(media library) +width
/height
(separate static inputs). This is whatAdaptedPropSource
was intended for, but XB's UI doesn't support this yet.we don't allow
width
andheight
to be entered either, because we cannot support it yet. Here too, that's possible, because both are optional 👍 -
Do you expect us to fetch the values and store/own them as prop inputs? Fetch the values and use them, but not store them? Do you want users to be able to provide their own title, width and height, that would override whatever oEmbed provider supplies?
Here too, everything I wrote in response to #9.1 and #9.2 applies:
width
andheight
are not the ones you seem to think these are referring to (and would requireAdaptedPropSource
in the UI), andurl
is the only one that is required.
tl;dr: forget about all optional key-value pairs, limit scope here to only the required ones, because they're the only ones that are feasible.
- 🇬🇧United Kingdom f.mazeikis Brighton
@lauriii
In #5 you've proposed shapes for
video
andoembed_video
and I have some questions.- For
video
you've proposedposter
property - as @chirstian pointed out on the draft MR - the commonly used term (and already existing property in Drupal Media API) forposter
would bethumbnail
. This also applies to Youtube, vimeo and others. Is there a good reason we would not usethumbnail
as terminology and the actual Drupal Mediathumbnail
property? - Properties for
height
andwidth
forvideo
are not currently stored for video_file source, we could alter the source and add missing properties viahook_media_source_info_alter
- is this what you would expect? - For
oembed_video
described properties could be fetched from oEmbed resource provider (Youtube or Vimeo) - Media entity itself seems to fetch those at the runtime. Do you expect us to fetch the values and store/own them as prop inputs? Fetch the values and use them, but not store them? Do you want users to be able to provide their owntitle
,width
andheight
, that would override whatever oEmbed provider supplies?
Depending on the answer to 3 it might look quite similar to 2 - just trying to figure out what the expectation is here, before I write a bunch of code.
- For
- 🇺🇸United States tedbow Ithaca, NY, USA
Note I have not had time to fix the phpunit tests but the e2e test passed so that is good sign. Just to need to update the phpunit tests to expect to a nested structure with revision info from the client.
- 🇺🇸United States tedbow Ithaca, NY, USA
@jessebaker I found the problem that is causing #18
It is because during Publish we delete the auto-save entry, including the hashes and the client instance id, for the items that were published. This is because there are no auto-saved information at this point. Everything matches what should have been just saved. We don't want this to show up "Review X changes"
So when the client sends the auto-saved hashes when find a mismatch, client has auto-saved hashes and the server doesn't. Now we could change the server side mechanism to stored this information separate from the actual auto-save information we will need to publish the changes but that would make the server side more complex. Also it is also possible that this particular entity will never be edited within XB again, so this keeps us from having to keep auto-saved hash information for any entity that was ever edited in XB forever.
So I was going to suggest that when submit a publish request and get response from the server that you delete the auto-save hashes. That would solve the problem in #18 but I did make me think of another problem, that actually exists right now
- User 1 open xb/xb_page/1/editor, does make any changes, there is no auto-saved hash
- User 1 leave their browser open
- User 2 opens xb/xb_page/1/editor makes some changes
- User 2 publishes xb_page 1
- User 1, makes a change in their browser session that they never closed
I think the effect would be that User 1 would make an auto-saved version that would not be aware of the published changes and so would wipe out those changes when it was published again.
We could solve this by not deleting the auto-saved entry the server-side, and therefore throwing a conflict error for User 1. But again this would require use to keep auto-save hash information forever for all entities, which could start to add up on larger sites.
So I don't think this is a solution.
I think what we need to do is have the client be aware of the starting point version of the entity it is editing.
So for example for a page or node this would be version number,
vid
orchanged
property.
Basically the server would return to the client, along with the "autosaves" key, a "verison_id" key of the last saved version on the server. The client would always return this key.So in the example above User 1's last request would properly get a 409 error because it's version number would be 1 version number behind. But this would also require the client to request
/xb/api/v0/layout/xb_page/1
after publishing. Though after 🐛 [Meta] Selective Publishing and Reverting Active the client could try to be more selective and not request if the current page was not publish it would probably be safer to always just request it.There are probably unrelated reasons why the client should always call
/xb/api/v0/layout/xb_page/1
after publishing. For example Drupal hashook_entity_presave
which allows altering an entity before it is saved. Of course XB will not be aware of any changes made in custom code here, therefore if XB doesn't call/xb/api/v0/layout/xb_page/1
after publishing the auto-save request could wipe out any changes that were made in anyhook_entity_presave
code