🇺🇸United States @traviscarden

It looks like this is ready for @mayur-sose. I'm not able to assign it to him directly, so I'll him to come claim it, and I'll assign it to myself in the meantime so no one else works on it at the same time.

Also, does this actually still need tests per the Issue tags?

Prefatory note: Let the record reflect that I wrote a masterpiece of a comment--a work of art, really--that will be forever lost to the sands of time and faded dreams because Drupal.org thought it would be funny to log me out when I hit "Save". Just take my word for it that the following is but a faint reflection of the veritable paragon of literary excellence that was my original composition. To wit...

I'm personally totally in favor of avoiding boilerplate by excluding some rules in XB's phpcs.xml 🤓 I'm curious what your thoughts are on that!? 🙏

My thoughts are good riddance! I'm glad we're in agreement, because I think they're clearly a net negative in test code--added overhead with no benefit whatsoever. Whoever thought they were a good idea may have them along with my pity.

All PHPCS tests pass now. The Cypress tests seem to have failed due CPU/memory issues. I'll restart the job and put this issue into "Needs review" on the assumption that the very minimal changes in this MR couldn't be the cause.

@denist3r, thank you for your contribution! Good catch on your second commit, in particular. I made sure you got commit credit. 🙂

traviscarden → changed the visibility of the branch 3511705-update-phpcs-config to active.

traviscarden → changed the visibility of the branch 3511705-update-phpcs-config to hidden.

👆 Rebased onto 0.x.

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

traviscarden → created an issue.

I'm not sure why this is in "Needs review" without an MR or apparent conclusion in the comments. I'll assume it's supposed to be "Needs work". Also, I may as well affirm, as long as I'm here, that the problem does, indeed, still exist in 0.x.

👆 Rebased onto 0.x.

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

The issue is still reproducible by following the steps in the description.

Makes sense; thanks. 🙂

The referenced video no longer exists; it was removed because it was out-of-date.

traviscarden → created an issue.

Well, let's try it and see if the tests pass, shall we?

Thank you, @amateescu; that was an important piece of knowledge.

When I created this issue, I thought simple config staging was supposed to already be supported, so I'm changing this to a feature request, and I'm presuming to mark it "Major" since it's in the critical path for Experience Builder.

The draft MR I created above adds failing tests that are basically complete for the happy path. They should be enough to prove that the basic functionality works. Then we can proceed to add tests for secondary functionality, edge cases, and error-handling, etc.

I'm going to move on to writing tests for other functionality in the meantime.

Huh, you're right. Curious. Well, this gets all the other instances I see.

Well, the oldest instance in the file came into Core that way-- #2784921: Add Workspaces experimental module → --and the second one looks like a copy/paste of the first one. Going back to the contrib module, it looks like it may be refactoring artifact from a very similar line that actually had side effects:

 $this->getSession()->getPage()->findButton($workspace->label())->click();

Compare the Core code in question:

$this->getSession()->getPage()->hasContent("$label ($id)");

I do think it's just dead code.

traviscarden → created an issue.

traviscarden → changed the visibility of the branch 3503414-allow-cms-author to active.

traviscarden → changed the visibility of the branch 3503414-allow-cms-author to hidden.

So the questions we're asking now are obviously much broader than the original scope of the issue--almost existential in some ways. There's a lot of--though not total--overlap with 📌 Decide on an approach for writing tests for OpenAPI integration Active . I'll go ahead and leave my analysis here until we decide to reorganize the work. For findability, so it's not just buried in the body of this comment, I'll refer to OpenAPI.Tools, which looks very useful.

So in short, what I'm hearing is that our API development is still risky and painful due to poor specification, testing, and quality controls, leading to regressions and surprise obstacles. Specifically...

1. Our OpenAPI implementation is incomplete, untrustworthy, fragile, sometimes invalid, and inconsistently enforced.
2. Our test coverage is likewise incomplete, inconsistent, and ad hoc.

In order to stabilize API development, we need to figure out how to...

1. Ensure that openapi.yml is valid, correct, and complete. That...
   1. All API paths have an entry.
   2. Each entry specifies all of its supported methods, e.g., GET and POST.
   3. Each method specifies all known possible responses, including error responses, e.g., 200 and 500.
   4. The strictest available data validation rules are applied consistent with design requirements.
   5. The file is as correct, readable, and idiomatic as possible.
   6. It would also be nice to enforce some consistent formatting and reduce merge conflicts.
2. Get it to actually be consistently enforced on API messages (requests/responses).
3. Ensure our tests are coextensive with it and both are complete.
4. And ensure that it all stays that way.

Here are my thoughts on solutions, taking for granted that they will still revolve around openapi.yml and PHPUnit:

1. We should consider static analysis that goes deeper than our PHPUnit tests of specification itself currently do. It looks like there are a lot of options out there. A good linter that runs on CI seems like an obvious opportunity.
2. We should consider whether thephpleague/openapi-psr7-validator is (still) the best solution for enforcing the specification.
3. We might consider publishing an OpenAPI documentation site generated from our openapi.yml. That would mostly likely prevent certain errors from creeping in, and it would probably have value in its own right, since anybody in the community could access it.
4. We can either write some PHPUnit "meta tests" that inspect openapi.yml and look for corresponding tests, or we can try to enforce test requirements with interfaces and test class design, or we can look into PHP attribute-based solutions to see if they can provide any help.
5. We can extract some universal but commonly overlooked test cases and centralize them. For example, instead of each developer having to remember to test for error-handling for invalid POST bodies, we could have one test that just iterates over the paths in openapi.yml or uses the autoloader to get all of our endpoint controllers and just posts garbage, asserting that it gets an error response for each one.
6. It may be possible, by extending some of the open source tools available (many of them have extension systems) or by writing something ourselves, to automatically generate some amount of openapi.yml code automatically.
7. There may be some opportunities to improve the design of the API (controller) classes to enforce some consistency--to reduce boilerplate and require a baseline of common functionality through interfaces and base classes.

That's my first round of analysis. If I were asked, I would probably suggest starting with a linter/static analysis tool, since we'd be likely to get so much out of a good one "for free". Then I would look at our validation library and either fix our implementation or replace it. After that, I would do whatever seemed the most fun at the time. 😉

Hopefully that's what you were looking for, @wim leers. Now's the right time to ask, right? When I've already done it. 😬 🙈

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

This MR is quite old and out of sync with 0.x. Let's start with a rebase. 👆

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

I'll start creating/working on some child issues, beginning with 📌 Add basic test coverage for `wse_config` with simple config Active .

I've added a WIP branch to show the current state of my work while I attend to another priority. I've got a basic framework (that can probably be extracted for more generalized use later). It's going well, but wse seems to break Core's workspace test utilities, since they work until I enable it:

TypeError : Drupal\Tests\wse_config\Functional\WseSimpleConfigTest::createAndActivateWorkspaceThroughUi(): Return value must be of type Drupal\workspaces\WorkspaceInterface, null returned
 /var/www/html/web/core/modules/workspaces/tests/src/Functional/WorkspaceTestUtilities.php:73
 /var/www/html/web/modules/contrib/wse/modules/wse_config/tests/src/Functional/WseSimpleConfigTest.php:99

That's where I'll pick up when I get back.

traviscarden → created an issue.

traviscarden → created an issue.

That's better. Thanks for helping me figure out the object serialization bit, @tedbow. The tests are passing now. On to @balintbrews for the frontend part.

Thanks, @wim leers. Quick status update:

1. Thanks for pointing out the pattern in `ApiLogControllerTest::testApiLogController()`. If Prophecy was a problem, it apparently wasn't the whole one, because I'm still getting the same "Serialization of 'ReflectionClass' is not allowed" error on CI. I don't see any indication of which test is causing it--unfortunately, because the new tests are passing for me locally.
2. I'm also using Drupal 11.1.x, yet other tests are failing even on 0.x--not the same ones as on CI.

Ted and I are going to look at this together this afternoon. Hopefully I'll be able to report progress then.

Well, I got PHPStan passing, but I'm getting completely different PHPUnit failures locally from CI. I'll assign this to @balintbrews until I come back to look at it again tomorrow in case he can make any progress on the frontend part in the meantime.

longwave → credited traviscarden → .

How about ^ this, @wim leers? It takes your proposal with two small changes:

1. Instead of testing for a specific exception class, it just checks for a ::getVerboseMessage() method and uses it if it's there.
2. It extracts the conditional local to a helper class.

wim leers → credited traviscarden → .

@wim leers, there were actually some pretty significant problems with our openapi.yml--most notably, schema-related specification violations. It doesn't surprise me it hasn't been doing everything we want it to. I've identified a few opportunities for improvement, but it took kind of a long time to get the existing file passing validation, so I haven't had time to actually implement them. Here's an overview:

1. Figure out why our tests didn't catch the specification validation errors on openapi.yml. I believe that's what OpenApiSpecValidationTest is supposed to be doing.
2. Specify all responses, including errors. We haven't been rigorous about specifying error conditions. I believe there are some ways to specify defaults or templates that could reduce boilerplate.
3. Specify parameter minimum, maximum, and default where appropriate.
4. Add allowEmptyValue where possible.
5. See if there are any other primitive schema types that can be changed from free values to enum because there are a definite, limited number of valid values.
6. Specify (serialization) style on array parameters. See Describing Parameters > Schema vs Content. I haven't looked closely at the places this would apply yet to see how much value it would actually provide.
7. Add missing descriptions. I added stubs for them with TODO values for now.

That's what I've got for now. I don't know what you want to accomplish within this issue specifically, @wim leers. Anything that doesn't seem urgent could be moved to 📌 Lay out a strategy for OpenAPI integration Active , if you like.

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

traviscarden → created an issue.

Thank you both!

traviscarden → created an issue.

Looks right to me!

Mmm. I'm not sure that got it. I've just reverted the commit and re-applied it with the correct attribution. Happy Thursday. 😄

Oops! I didn't get you credited in the commit message, @thejimbirch. I think it'll pick up on the fact that you authored it. Let's credit you in a comment, too.

Cool! Fixing a few (mostly grammar) mistakes in the UI text and merging. Thanks!

I assume this should go back to @tedbow now.

traviscarden → created an issue.

Just the usual little formatting niggle and it looks good, @tedbow.

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

Ooh, interesting! I was sick all last week, so I haven't had a chance to look at this. I'll try to check it out soon.

@tedbow, for some reason I can't seem to create an MR. The button's just absent from the UI. (I feel like we've had a problem with this before, and it might've been a permissions issue.) In any case, I've pushed a branch with the fix. Could you see if you're able to create an MR and then review it?

traviscarden → created an issue.

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

Oops. You asked me to go ahead and merge. Done. 🙂

Looks good!

Congratulations on your first Core commit, @secretsayan! 😄

Approved!

wim leers → credited traviscarden → .

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

Thanks, @jessebaker; this is huge. 😉

traviscarden → created an issue.

Nice. 🙂

Fix broken links to config files.

traviscarden → created an issue.

traviscarden → created an issue.

I've marked 🐛 Canvas flickers when adding a component to an empty slot Active as a duplicate of this issue and added the screenshot and additional detail from it to this issue's summary. I don't know if they have the same root cause. If they turn out not to and can't be easily solved at the same time, we can re-open that issue.

That makes sense, @lauriii. I could certainly imagining the root cause being the same. I'll close this one as a duplicate and add the added detail to that one. Thanks.

@secretsayan asked me to push this change for him real quick because he's in the process of moving to a new laptop.

Thanks, @wim leers. I agree with your assessment. I think the original impetus of this issue was based on a misunderstanding, and the value to be derived now is just findability in the short term. I've updated the summary accordingly.

Also, like you, I assumed that adding the local task would be a matter of minutes, but I must be missing something. I can get a local task to appear if I hard-code the node ID, but I can't get it dynamically. Anything I try results in either a fatal error or just not showing the task. I'll create an MR with the hard-coded nid. I need someone to help me figure out the last part.

traviscarden → created an issue.

traviscarden → created an issue.

Thanks @omkar-pd; that fixes the presenting problem, but apparently it was supposed to be red.

I think that's intrinsic to the scope of this issue. Nodes are the presenting question (and the answer seems obvious to me). I assume most other entity types already have their own conventions--Views, blocks, media, etc. Maybe we should break this issue up a little bit: 1) keep this one but scope it at Nodes only, 2) create one for deciding on other entity types when we're getting ready to implement them, and 3) expose the ability the define the behavior in the API. What do you think?

traviscarden → created an issue.

traviscarden → created an issue.

Lol. "felxibility" 😅

traviscarden → created an issue.

wim leers → credited traviscarden → .

Nice! I prettified openapi.yml and approved it.

Re: #29

🟢 Scenario 5.1: Deleting created entities when abandoning progress

There should be a way to delete the draft by using a delete action but once the user has entered the content form and inserted any content there, we should retain the saved draft.

I don't understand. I assume "saved draft" means an auto-save state. Is that right? What do you mean by "content form"?

🔵 Scenario 6.1: Deleting created entities when abandoning progress

There might be different expectation between [Media entities and taxonomy terms]. [In] the use case of Taxonomy Term because… you're just adding a tag inline using an autocomplete.

But you can also configure Taxonomy term fields to "create referenced entities if they don't already exist". The point is that it’s possible to create separate entities from a node edit screen. Does the rest of the scenario look right?

🟡 Auto-save only applies to saved entities. It doesn't make sense in any other context, e.g., an un-submitted node/add/* form, since (it is also assumed) Experience Builder can't even be opened except on an existing entity.

I don't think this is the right assumption. Content types that are using Experience Builder should open in XB even when creating content.

Okay, so when you create an XB-enabled node, for example, there is no standard node/add form at all? You go straight to Experience Builder and that's the only way to interact with the entity? If so, that should be added to the assumptions. And we probably need a new scenario for editing the overall entity information--such as title, URL alias, and menu settings--from within Entity Builder. Would you agree?

🟡 If a user wants to revert back to a specific point in history, that will be handled with revisions.

It should be possible to revert back to previous revisions and auto-save states. It would be okay if auto-save states get truncated into a single revision once content gets published.

Wouldn't that mean that the "truncated" (meaning, squashed or merged, I assume) auto-save states are always identical to the list of revisions--making them effectively useless? Wouldn't that mean that every revision would have an identical auto-save state?

🟡 What happens if the same user has the same node open in Experience Builder at the same time on two different computers? For example, what if a user goes back and forth between home and the office? Does one computer get locked out of editing and become "read only"? How do we avoid the two computers repeatedly overwriting each other's auto-saves?

We could do something similar to: https://www.youtube.com/watch?v=CsRBypaT_hM&t=27s. We could apply this both for the scenarios where the user is the same and where it's different.

Given the workflow depicted in the video, I suppose that would mean a user on computer two would have to request control from computer one. And ignoring the logical challenges with that, it would mean that our auto-save states would no longer be associated with a user but rather with a browser session, right? Otherwise the concept of locking doesn't make any sense and we haven't solved the problem in question at all.

🟡 What happens to one user's auto-save states if a node or revision is changed by another user?

Now that I understand that only one user can be working on a single entity at a time, problem can't actually materialize, then as long as starting an editing session in Experience Builder locks the whole entity, this is irrelevant.

"Wounded." Haha. No, it was actually dead. As I understand @tedbow per a conversation we had outside this issue, it was removed on purpose and put back in by accident during a merge or rebase. Trust us, @wim leers : we don't make mistakes around here. At least not when we're correcting our mistakes. 😉

A lot of the details in this issue (that assumption included) are pending confirmation from @lauriii. See the legend for interpreting the status markers.

Granting our assumptions, it seems like we're more or less in agreement on the proposed resolution at this point. (Tell me if I'm wrong.) I'll see if I can get @lauriii's eyes on the issue, and in the meantime, I'll find a good place in the codebase to document our architectural direction. Since the scope of this issue is research, that should resolve it, and we can create follow-ups to start on implementation.

The wording of these user stories assumes that the entity has already been explicitly saved (i.e. exists as an entity). autosave_form also handles the case where nothing has been explicitly saved ... I don't know if that is an XB requirement but it would be good to explicitly rule it in or out, currently it's not mentioned.

Good thinking, @catch. I assume it is not a requirement since, as far as I know, there's no way to use Experience Builder except on a piece of content that already exists.

I've updated the issue summary to include this assumption.

Is that a question, @wim leers? 🙂