🇺🇸United States @traviscarden

We're going to break this into multiple issues. Marked postponed until we get a chance to do so.

I started to create a static helper to centralize the path generation logic (because it is a little inconsistent), but most of the API path strings were in YAML and JavaScript files, so once I decided not to use it in test classes (because it would make them less expressive), there weren't enough uses left to justify its existence. I'm basically explaining why I didn't have an MR in the 5 minutes it took to do a search-and-replace. 😛 Anyway, here it is. ^

Tagging this "Experience Builder" since it's desired (if not eventually required) functionality there. Thanks for your work on this, @adamzimmermann!

Thank you, @smustgrave. This hasn't affected my work for over a decade at this point, so since it never gained traction, I'm happy to go ahead and close it now.

On it, @wim leers!

traviscarden → created an issue.

I guess that makes sense, @fjgarlin, since these are the first negative terms--in other words, dictionary.txt was probably already being scanned, but it always passed because it contained all of its own terms. 😛

So the tests pass now. Bumping priority since this affects APIs, and we don't want to start to drift and get out of sync. On that account, I'm going to go ahead and assign directly to @wim leers to see if we can rush it through. Ready for review!

I think I got all but one thing, @wim leers: Where should we document the standard? I didn't see a place that it seemed like it fit.

I've updated the issue summary to clarify that I mean to let the dev team know about it after it's been committed, just to prevent surprise and confusion.

Also, I want credit for the fact that my initial analysis in the issue summary already included the issue queue component. I don't want that counting against me if you and I ever get into some kind of contest, @wim leers. 😉

Finally, I see the cpsell job failed. Am I crazy, or did it fail because it found a forbidden word in the directive with which I forbad it in dictionary.txt? 🤨 Well, maybe it's an easy fix--maybe the inline comments confuse it... but it's time for me to log off for the night. @wim leers, maybe you know the solution off the top of your head.

I tested it, and it works now. Thanks, all. 🙂

Concerning spelling, @wim.leers, ordinarily I go with the primary spelling provided by Merriam-Webster dictionary. If it's not in the dictionary or it has a more domain-specific use, I go with the most authoritative reference I can find or, if there isn't a clear authority, favor the closesst Wikipedia article title.

In this case, I followed the precedent in the nearest code examples, but upon closer inspection of the whole codebase I see we have overall favored the hyphenated form. I've created 📌 Decide between autosave and auto-save and standardize Active to discuss standardization.

traviscarden → created an issue.

traviscarden → created an issue.

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.