I am going to mark this as RTBC since @larowlan approved the merge request
UPDATE:
After I wrote most of what's below I thought, maybe I am overthinking it. Hopefully when we use Workspaces, our "publishing" will just be moving entity revision to another Workspace, and we can rely on Workspaces for handling transactions. If that is the case maybe we could just surround the current code in a transaction.
but I wonder if one quick improvement to make a deadlock in the tempstore less like would be to introduce \Drupal\experience_builder\AutoSave\AutoSaveManager::deleteAll()
which would call a new \Drupal\experience_builder\AutoSave\AutoSaveTempStore::deleteAll
and then that would call the existing \Drupal\Core\KeyValueStore\KeyValueStoreInterface::deleteAll()
I don't know that much about tempstore deadlocks but it seems like this \Drupal\Core\KeyValueStore\DatabaseStorage::deleteAll
is 1 db operation so hopefully that would decrease our chances of deadlocks
Original, maybe overthinking🤔
I think this is good idea but also I wondering if there are thing we could do to make a deadlock more unlikely
However, there more may likely be deadlocks on the temp store when deleting the autosave.
Could we avoid this by making sure our tempstore is not read during the publish process?
For instance since currently you can't select which items you will publish, you have to "Publish all", could we stop the XB UI from working once the "Publish all" process is started. Nobody should really be making changes once you click "Publish all". Before we start saving the entities we have already loaded the auto-save states, $this->autoSaveManager->getAllAutoSaveList()
, into memory so there is not chance new changes will affect the entity saves but another user using the XB UI would have no way of knowing that.
Maybe something like this
try {
$this->connection->startTransaction();
$this->autoSaveManager->lock();
foreach ($entities as $entity) {
$entity->save();
}
$this->autoSaveManager->deleteAll();
}
catch(Exception $e){
if (isset($transaction)) {
$transaction->rollBack();
}
Error::logException($this->logger, $e);
throw $e;
}
Then maybe the controllers that interact with the AutoSaveManager
could do something like this at the start
if ($this->autoSaveManager->isLocked()) {
throw new Exception('Experience is currently publishing.....');
}
We could put this a trait method, ensureNotPublishing()
.
Otherwise I don't think there is anything currently stopping somebody from continue to make XB changes while another user has just clicked "Publish all"
@wim leers thanks for finishing this up!
wim leers → credited tedbow → .
need to resolve merge conflicts
I think this is ready for review now
wim leers → credited tedbow → .
I got sidetracked by #33 but did push this forward and address some of the points.
@wim leers if you want to wait till give this another self review that is fine too
There are more problems with XbConfigEntityHttpApiTest
and our openapi.yml
see
#3505224-6: XbConfigEntityHttpApiTest missing test coverage for individual GET requests for JavaScriptComponent + AssetLibrary →
there are also big problems with our test coverage for non-empty list responses in `XbConfigEntityHttpApiTest`
\Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testAssetLibrary
doesn't have test coverage for a non-empty response from\Drupal\experience_builder\Controller\ApiConfigControllers::list
Trying to add test coverage for this in ✨ Auto-save code components Active I found it is not possible because the response has an openapi validation error.
We have
description: All available asset libraries content: application/json: schema: type: array
But the "array" schema I don't think works because Looking at
\League\OpenAPIValidation\Schema\Keywords\Type::validate
it hascase CebeType::ARRAY: if (! is_array($data) || ArrayHelper::isAssoc($data)) { break; } $matchedType = $type; break;
So it doesn't match "array" for associative arrays.
\Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testJavaScriptComponent
does seem to test a non-empty list response but the openapi spec for/xb/api/config/js_component
doesn't have GET specified so there is no openapi validation for the response\Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testPattern
does seem to have test coverage for non-empty list responses but Looking atpaths./xb/api/config/pattern.get
in our openapi there is
patternProperties: '^\\[a-zA-Z0-9_-]$': $ref: '#/components/schemas/PatternPreview'
So because of 🐛 Openapi.yml uses patternProperties which is not supported by our dependencies Needs work this doesn't actually trigger validation
I have confirmed this by switching the above to$ref: '#/components/schemas/LayoutSlot'
andtestPattern()
still passes 😭
So I think they are all broken in a different way 😱
Would be a good idea to refactor this test? looking at testPattern()
don't all the entity types need to test the same things
- Authorization, already refactored into
assertAuthenticationAndAuthorization
- Failed POST for Openapi validation
- Failed POST for requests that pass Openapi validation
- empty individual GET and empty list GET before entity creation
- Successful POST
- non-empty individual GET and non-empty list GET after entity creation
- 409 error for trying to POST another entity using the entity id key
- Failed PATCH for Openapi validation
- Failed PATCH for requests that pass Openapi validation
- Successful PATCH
- DELETE
- empty individual GET and empty list GET after entity deletion
Maybe this not the exact list but whatever the list is shouldn't be same for all entities?
Could we maybe make 1 test method like
public function test(string $entity_type_id, array $openapi_invalid_data_cases, array $entity_invalid_data_cases, array $valid_initial_entity, array $expected_entity, array $valid_update_entity, $expected_updated_entity);
For
- Failed POST for Openapi validation
- Failed POST for requests that pass Openapi validation
- Failed PATCH for Openapi validation
- Failed PATCH for requests that pass Openapi validation
we could use array $openapi_invalid_data_cases, array $entity_invalid_data_cases
for both cases
I think this would force use to have the same coverage for the all entity types.
If that is too many arguments to a test method we could also do,
public function testOpenApiValidation(string $entity_type_id, array $invalid_entities, $valid_entity);
public function testEntityiValidation(string $entity_type_id, array $invalid_entities, $valid_entity);
re #29
I found another problem with the existing test coverage 🐛 XbConfigEntityHttpApiTest Active
I think I started testAssetLibrary()
and probably copied the existing testJavaScriptComponent()
as starting point . So it makes sense that since testJavaScriptComponent()
didn't test this testAssetLibrary()
would also miss it
Working on the test changes I found this 🐛 XbConfigEntityHttpApiTest::testJavaScriptComponent doesn't change the component on the first PATCH request Active . would be good to get that little fix because we are updating that test function in this issue, so it is bit confusing. Because we would also want to ensure the PATCH doesn't affect the auto-saved version
Maybe I missing something but I think was just oversight
tedbow → created an issue.
I think the clients existing call to /xb/api/autosave
which uses ApiPendingChangesController
could be used to if there are auto-saved changes. I am unclear if the client already has info about " current page has been published previously". but if it does there should not be need to be any back end changes.
This would need to be solved though 🐛 Once previewed in XB an entity with no changes will still show up in "Review x changes" Active
Looks good,
perform any rituals of passage he'd like 🤣
Preparing the rituals.....
but for now forgot how to perform the ritual that by-passes gitlab approvals, so will just try to get them 😜
@wim leers thanks for the review!
I am working on the need for test coverage changes https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
but pushed back/clarified on 2 other points. Assigning to you for the 2 points I disagreed but if want to wait on the test changes first thats fine
We might want to update `ApiPendingChangesControllerTest` to handle an Code Component in auto-save.
working on this
added openapi and 403 tests
Actually I still to test the openapi expected errors
I thought about handling cleaning up code components auto-save state if the config entity was deleted but I realized that is problem across entity types so I created 🐛 ApiPublishAllController does not handle since deleted entities Active
Thanks again for the help. Turned out it was other software installed on my laptop that was blocking certain requests
So I made another clone of Drupal core without Experience Builder and something similar is happening
If I run \Drupal\Tests\node\Functional\Rest\NodeJsonCookieTest::testPatch it fails in a similar way.
{"current_user":{"uid":"2","name":"w8ftf0xl"},"csrf_token":"mXeGJQEHIPybY_yeVvwuZSO3t4I6QGxNBgoBWDP9Vfw","logout_token":"o4SO3C55Y6Z7f9GThYpeGPvwtyajb1DcObPC7NBFl0A"}
Failed asserting that 200 is identical to 405.
/Users/ted.bowman/sites/xb-2/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:906
I simply changed a line in to show the body in EntityResourceTestBase. so the body on assert fail
$this->assertSame(405, $response->getStatusCode(), (string) $response->getBody());
so I think my findings in #10 were wrong about an asynchronous request. could it be that we making a patch request Drupal somehow redirects to the session route?
😱💩
Well I
- uninstalled Laravel valet and nginx
- switched to apache using https://getgrav.org/blog/macos-ventura-apache-mysql-vhost-apc
- rm -rf vendor + composer install
- restarted brew services restart httpd with every step
- run the tests from phpstorm and from the command line
../vendor/bin/phpunit ../modules/contrib/experience_builder/tests/src/Functional/XbConfigEntityHttpApiTest.php
still the test fails in the same way with "WTF, why is the response the token?"
- Thanks all. I am using Laravel Valet, which uses local php and nginx. I may switch back to https://getgrav.org/blog/macos-ventura-apache-mysql-vhost-apc
- The other thing I have noticed when step debugging.
In this block of code, with xdebug on and put a breakpoint lines 3 and 4 and put a breakpoint in the first line of \Drupal\experience_builder\Controller\ApiConfigControllers::patch
1. $token = $this->drupalGet('session/token'); 2. $request_options['headers']['X-CSRF-Token'] = $token; 3. $response = $this->makeApiRequest($method, $url, $request_options); 4. $body = (string) $response->getBody();</li>
when I stop at line 3 for patch request and then press "resume" I hit the breakpoint in ApiConfigControllers::patch but also the test advances to line 4 even if leave the server paused on
ApiConfigControllers::patch
. So it seems the request is asynchronous even though\GuzzleHttp\Client::request
sets$options[RequestOptions::SYNCHRONOUS] = true;
Anyways I have wasted too much time on this. I am probably going to switch back to using local apache
hooroomoo → credited tedbow → .
On the test I am writing for
✨
Auto-save code components
Active
it also fails in the same way locally on a PATCH request. Not sure if this a coincidence. But in both these cases \Drupal\Tests\experience_builder\Functional\HttpApiTestBase::assertExpectedResponse
is being called at least once before this with no problem.
tedbow → created an issue.
I have started on this
Trying figure out sizing for this issue as far as what is needed for the back-end:
I see that ✨ Editing code components Active was just committed so there might be some concretes ideas of what the front-end expects from the backend now.
It seems like from discussion on ✨ HTTP API for code component config entities Active that we won't be implementing auto-save in in manner like content entities where the front will use something `\Drupal\experience_builder\Controller\ApiPreviewController` that returns a render markup and as a side effects does an auto-save, correct?
Instead we will be using a more direct method where the client will be sending up exactly the original and compiled js and css that it wants in the auto-save, correct?
I think if that is that case the auto-saving might be able to be a simpler controller wrapped around \Drupal\experience_builder\AutoSave\AutoSaveManager
Basically we would need a PATCH and GET(for reloading the editor) for the individual entity as saved in the AutoSaveManager
does that sound correct?
I think to find the number of pages or content entities in general that use a specific component we might need 📌 Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases Needs work or more likely 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active
Also one thing to note, 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active won't directly get us the number of instances as it would just say if it was used at least 1x on a content entity.
from the summary
Display information about pages that currently use the code component.
Is it good enough to show how many pages use the component but not how many instances of the component there actually are?
So if a component is used on 2 pages but on 1 page it is placed 3x should the number shown be 1 or 4(1 page 1x + 1 page 3x)?
Also we want show indirected uses, correct? Meaning if Component A is used on 1 page by placing the Component A directly on the page and another page by placing Component B that imports Component A, that would show 2 uses, correct?
tedbow → created an issue.
With my latest commit PropSourceEndpointTest
should pass. Other test for the new stuff in this MR will probably break but a least we know $library_parser->setActiveTheme($active_admin_theme);
is likely messing with the caching
🎉Ok figured out which line makes PropSourceEndpointTest, experience_builder_library_info_build()
pushed up a commit with this. But will paste it here:
// 💡 Returning here will make PropSourceEndpointTest pass. If \Drupal\experience_builder\Asset\XBFriendlyLibraryDiscoveryParser::setActiveTheme
// is called in the next line,and we still return [], the test will fail.
// I guess this results in a call to \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme().
// Could it be that calling loadActiveTheme during the hook makes the uncachable?
//return [];
$library_parser->setActiveTheme($active_admin_theme);
// If we comment out the return above and make it here PropSourceEndpointTest will fail.
return [];
I am not familiar with \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme(). but assume it usually done 1x a request. Maybe calling in the hook again with a different theme makes the pages uncachable?
I chatted with @bnjmnm about this. PropSourceEndpointTest failed for locally that same way it does on gitlab. It passes for me locally on 0.x.
I tried various things and the only thing that worked was reverting that changes to experience_builder.redux_integrated_field_widgets.inc
I restored and put some debug asserts locally to see if I could figure out what was happening
This is the assert after the 2nd request for xb/api/config/component
// Ensure we get the exact same response contents as the first time.
$secondResponseDecoded = $debugGetDecodeResponse();
$this->assertSame(array_keys($firstResponseDecoded), array_keys($secondResponseDecoded));
$this->assertSame($firstResponseDecoded, $secondResponseDecoded);
The last assert fails. Since the first passes we know the same the components are returned in the list
It is just that differ
It turns out components for blocks like block.user_login_block have different default_markup
so
- <input data-drupal-selector="form-qre8a3p5fsuun59wo6jradkbpeceewp-2ez9swbej9m" type="hidden" name="form_build_id" value="form-qrE8A3p5FSuUn59WO6jrADkbPEcEEWp_2EZ9sWbEj9M" />\n
+<input data-drupal-selector="form-69vyw7nxd-eczqvemkgxhhdjy43obmtmxbgbicdmtdc" type="hidden" name="form_build_id" value="form-69vYw7nxd-ECzqveMkGXHHdJY43obMTMXbGbiCdMtdc" />\n
so the identifier for the form. I guess since cache was not hit the form was regenerated. I guess maybe that is not very helpful 🤷
Starting looking at the issue. I am not clear why the schema in the summary using a mapping for CS and JS with sub elements for source and compiled
js:
type: mapping
label: 'JS'
# The asset library may contain only CSS, no JS.
nullable: true
mapping:
original:
type: text
label: 'Original JS as entered by the human author'
compiled:
type: text
label: 'Compiled JS code, compiled from the original'
where as in the just finished ✨ Config entity for storing code components Active we use
source_code_js:
type: string
label: 'JavaScript Source code'
source_code_css:
type: string
label: 'CSS Source code'
compiled_js:
type: string
label: 'Compiled JavaScript'
compiled_css:
type: string
label: 'Compiled CSS'
My guess is there is not particular reason for the difference. The mapping might make more sense but I think consistency is important, so I am going to go with what we used for experience_builder.js_component.*
unless someone lets me know why these should behave differently
wim leers → credited tedbow → .
@catch my general concern from #11 that you were responding to in #15, is not an issue for me if we implement #12. Basically just only do this for demo_mode
, which I think practically this means only do this for Drupal CMS.
I agree with this for someone that installs the alpha module themselves.
So if this only affects demo_mode/Drupal CMS then I think others can use the alpha module like any other alpha module
...Trying to create an ephemeral demo on what are supposed to be production installs is just inviting catastrophic data loss or inability to update, in the various myriad forms those issues could appear.
I haven't been involved in the discussion around but I can see your point.
Is there another issue where I can see the discussion?. If demo_mode
is done I want to see if there is anything we can do to lesson the risk in XB itself
working on this
Need to check demo_mode
and update demo mode description
I chatted with @phenaproxima about this. It seems like if the script is updated to only uninstall if experience_builder.demo_mode
is true, then wiping with each update is ok.
✨
Add a code-only (not changeable via UI) "demo mode" flag that Drupal CMS can set and that future XB MRs can use to constrain XB functionality
Active
explicitly added this for Drupal CMS so I can't imagine others will use it. It seems like it is up to Drupal CMS to provide the message around.
Just to be careful I think we should update the description of demo_mode
in config/schema/experience_builder.schema.yml
to explain this behavior. This will avoid people finding this setting and not understanding what the implications of setting is, and this issue is definitely adding more implications for setting this flag.
-
I agree with #10
I think MR works well for Drupal CMS but it seems like getting testers for XB to be a pain.
This seems like a general problem that if you are using a Alpha version of the module each update is going to risk problems, maybe very bad ones, when you update the module. But that seems to risk you would also take with using any alpha module.
Say we have testers that are using Alpha1 they have made some complex pages, global templates changes, global css, and code component changes.
They find a small UX bug and we fix it in Alpha2. They want to test Alpha2 and give feedback. By default they would have to rebuild everything that they had already done. what if we they rebuild everything and bug is not there. Is it not there because fixed in Alpha2 or is it not there because when they rebuilt everything they did it just slightly different and they didn't hit the bug and won't have hit in Alpha2 either.
Upgrading between Alphas doesn't guarantee that we will break the data model and your site won't work. It just means it might. If we did this I can't imagine anyone would try to do anything complex with the alpha versions of XB, especially after we wiped their data the first time(maybe they didn't pay close attention during composer update). I think we do want risk takers to try complex with XB. That we would more confident to move onto alphas
- what happens if someone updates XB with Automatic Updates? They wouldn't see an option to not uninstall even if we added, right? I could see there being a large intersection of people who don't want to deal with Composer, so they use AU, and those who don't want to deal with twig templates, so they use XB.
Look at test errrors
Long-term, I think this ought to be implemented as \Drupal\Core\Config\ConfigFactoryOverrideInterface.
Are we sure long term we will need this at all if rely on wse_config
?
We have requirement out auto-save js components but this seem like more a UX requirement than a specification for how this would handled as far as backend storage. Could we just do real config saves, to back up work without user action and just let wse_config
be responsible for making sure that doesn't pollute the live site? (relating
🌱
Identify roadblocks to staging config in Workspaces (e.g., via wse_config)
Active
to this issue for that reason)
I think in 🌱 Research: Possible backend implementations of auto-save, drafts, and publishing Active the 2 main reasons we decided we needed some auto-save mechanism is that not a real entity save was
- Bypassing Validation: saving work in progress that is not valid
- Avoid the saving overhead so we could save frequently
For 1 we could still use a simple auto-save if the validation does not pass but a real entity save if it does. In the case of saving content entities we are not using the auto-save state, which might not be valid, to then render the content entity outside of the XB UI itself or to render that entity in the XB UI for another entity besides itself. Would that be the case for the JS components? What would even happen if we tried to a render JS component config entity overridden by ConfigFactoryOverrideInterface in XB UI for another entity, say Page content entity, if the auto-save state is not valid?
For 2 do we have the same overhead concerns for JS Component config entities as we do for Content entities. If XB is going to be the default way some sites add new and edit existing Content and as well as JS Component, it still seems likely that sites will not be adding as many JS components as they would content entities.
benjifisher → credited tedbow → .
quietone → credited tedbow → .
I think to find the number of pages or content entities in general that use a specific component we might need 📌 Ensure querying JSON nested values when parent keys are unknown is possible in all supported databases Needs work or more likely 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active
I will research this further
I am wondering if it would make sense to start an MR hear that would add a ComponentUsageCalculator
service. Because we are going to need some way to find how many usage particular component has. I guess this will be across content entities using a Json field query, but also we would need to loop take into account instances inside `PageTemplate` .
For PageTemplate entities is it just important show a list of which regions would be affected? Would this only for the active theme? I can imagine in some sites they make start with 1 theme, places some code components in regions but then decide to switch to another theme and not bother to uninstall the other theme. But in other sites they may have custom code or a module like https://www.drupal.org/project/theme_switcher → switches themes.
Clicking view swaps the modal to a deeper view of which
pages are affected
What about if
- You have 2 existing code components, A & B
- the code component B is used inside code component A
- Code Component B has been edited but Code component A has not
Would the "deeper view " show that Code Component A has been affected? Would it should all pages where code component A is used but not B directly? Because these pages would have changes resulting from B changes
Would the number 46 in the example image take into account all Component B placements?
This number shows the total number of instances that will be affected by the basic card component update.
So is this if you are making a new component or editing an existing it shows how many times the JS component is place
So for example:
- New or existing Component, but no instances in Unpublished Changes or already published = 0
- Existing Component
Unpublished changes: 2 instances added in Page A, 1 instance added in Page B
Already Published: 3 instances in Page C,
= 6 - Existing Component:
Unpublished changes, 1 instance added in Page A, 1 instance removed in Page C
Already published 3 instances in Page C,
= 3? because there will be instances once "Publish all" is submitted?
I made issue to specifically deal with 📌 Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active
Just a start but I will unassign myself as others will be starting there day tomorrow before me.
I bumped it to major only because I think we should decide sooner rather than later if this going to need changes to the system outside of Code Components. I made possible way we could do this that might just apply to the Code Components code
tedbow → created an issue.
Will work on this
I was just on call with @balintbrews and @lauri regarding auto-saving Code Component. I just wanted to get my understanding down in regard to the usage of auto-saved components, because this would intersect with out parts of XB.
This is my understanding at least for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active
- If a code component is only in an auto-save state and has never been published, it will not be available in the component list in the library of the XB ui. Meaning you can't place an code component on the page/node if it is only in auto-save
- if a code component has been published, it will be available in the component list in the library of the XB ui, it can be placed
- if a code component has been published but the editor is currently making changes that are in auto-save, the version available in the component list in the library of the XB ui that can be placed and the version for instances that are already placed, is the published version and is not affected by the auto-save state. This auto-saved version of Code Component also would not affect any instances on page/nodes/global regions that are already been "Published"(in the XB process of "publish all")
- for an already published version of a Code Component, if the editor has an auto-saved version, once the auto-saved Code Component is published all versions in both pending XB page/nodes/global regions, and already published XB page/nodes/global regions would use the updated version. There would no option to use the previous version
@Wim Leers re #54 ok. yeah I reread the issue in 📌 [PP-2] Don't store page template model data in auto-save for an entity Postponed . I missed this part
The values are filtered out in \Drupal\experience_builder\Controller\ClientServerConversionTrait::clientModelToServerProps but we are storing more in the autosave/tempstore than we need to.
I thought there would be conflict in "Publish All" phase because each entity could have its own copy of the global regions that could conflict. Now I see it is just extra data we don't actually use. So +1 on moving it to "Polish" section
Finished going through issue queue for 6. Save (draft) content section
I think we are close but we mind unknown issues once ✨ [PP-1] Implement the "Publish All" button Postponed is completed as they will allow use to do the whole process. For this to include entity fields we will need the 2.1. Content editing of meta fields section to be done too
As far as know is 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active isn't required for 0.3.0
FYI 📌 CONTRIBUTING.MD is nearly impossible to follow without insider drupal knowledge Active . I am not following either of these but so not sure if they are dealing with the same sections of the file
I merged this because it was random fail in another e2e test. Since this MR adds 1 new e2e test, not failing, I think it is fine to go in.
Will make an issue listing known random e2e tests
Renaming the title case in body sees this in the queue, to give a better idea of what it does
component-operations.cy.js
failed but since we just added a new test I can't see how this is related
retesting
Added issue for the "Save (draft) content" section. I am still looking for more issues to add
Assigning to myself for reminder but if anyone else wants to make follow-up MR please assign yourself. You can assign back to me for review
I assigned @traviscarden to review because we needed openapi.yml changes but now those have been fixed in 🐛 open #/components/schemas/LayoutPreview should require properties Active
So @jessebaker and myself already approved this
ok might have been something local
tedbow → created an issue.
I am not not really sure why this pipeline which just has new expection thrown https://git.drupalcode.org/project/experience_builder/-/jobs/4018805
Does report the error response
where as this one https://git.drupalcode.org/project/experience_builder/-/jobs/4018805
which was from the other issue and the reason I made this issue does not
Anyways bumping down to minor because this just might be me not understanding our e2e jobs will log errors for requests(not tests) that fail