🇺🇸United States @traviscarden

I think ^this gets the job done, @balsama. A couple of points:

1. This definitely preserves the existing functionality; and it seems to follow that it should work with a customized path, but I don't know how to do that (customize it) in order to test it--which, of course, makes me nervous. (It also makes me uncomfortable that we don't have a working JavaScript test suite, but that's a much bigger problem.)
2. The application doesn't handle errors very gracefully at all--but that's a pre-existing problem that's well beyond the scope of this issue. I just thought it was worth mentioning in this context since there are a few more places in the code now for errors to occur since we're making HTTP requests and parsing responses.
3. I added a commit to update the caniuse-lite yarn package. I know that's completely out of scope, but I got tired of seeing the warning every time I compiled. It doesn't hurt my feelings if you want to remove it from the MR.

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

traviscarden → created an issue.

I propose we remove the dependency altogether. I've successfully tested with with Gin 4.x and 5.x, Claro, and even Olivero, and it works fine in all of them. (It's squished in Olivero, of course, but it still works; and if you're using Olivero for your admin you're probably already collecting rent money from the voices in your head.)

In fact, it works so well in Claro, most people would probably be hard-pressed to tell the difference. Since that's the case with the default admin theme in Core, I don't think it's even worth mentioning Gin, much less putting it in hook_requirements().

Assuming we all agree on that, we just need to make one other decision: do we completely remove the Gin requirement from composer.json or not? It would technically be a backward compatibility break, because someone could have the theme installed solely as a transitive Composer dependency (i.e., because they've required JSON:API Query Builder and it in turn requires Gin). If we just remove it from our requirements, it would suddenly disappear from that person's codebase when they update. But I don't know if we have any sort of BC commitment. So...

1. If we need to preserve backward-compatibility, we should just loosen the Composer requirement so that it also supports Gin 5.x (the existing MR).
2. If not, we should remove the requirement altogether (a new MR I've opened).

I've removed the hook_requirements() implementation in both MRs, so unless I'm missing something, I think we're ready to just choose between them.

Technical notes:

1. There is a js/react-app/src/gin.css and corresponding HTML classes in the frontend code that seem to imply Gin support, but as far as I can tell, they're not actually specific to Gin (they create no implicit dependency), and they didn't cause any problems in any of the other themes I tested. There's nothing about them in the documentation or the commit log. I'm guessing the naming convention is just an artifact of AI oddities. I suggest we just leave them in there until and unless we a more rigorous audit/clean-up in the future.
2. I deleted the entire jsonapi_query_builder.install file in both MRs, because all it had was the hook implementation. That will probably cause a merge conflict with ✨ Replace JSON:API Schema dependency with Open API JSON:ARG module Active , but I think there would've been one either way, and it will be trivial to resolve.

traviscarden → changed the visibility of the branch 3537554-remove-gin-dependency to active.

traviscarden → changed the visibility of the branch 3537554-remove-gin-dependency to hidden.

traviscarden → changed the visibility of the branch 3537554-add-support-for to active.

traviscarden → changed the visibility of the branch 3537554-add-support-for to hidden.

Note: I have asked @ankitv18 to leave a comment just so the issue credit system knows he contributed.

I don't know why the tests are still failing on CI. They're all passing locally, and so is stylelint. I've been instructed to go ahead and disable the failing jobs so we can merge the MR. I have done so, and of course, the build passes now.

I have extracted @ankitv18's comment in #15 to its own issue: 🐛 Errors when sorting Active .

Handing off to @balsama for review and merge.

traviscarden → created an issue.

I've redone the local work (I want to make sure I didn't mess one of them up before; I'm not certain.) Here's the update:

• manual_test.htm (screenshot): Saved from a manual browser visit.
• custom-debugging.htm (screenshot): Captured by the test using $this->getSession()->getPage()->getOuterHtml().
• simpletest_browser_output.html (screenshot)

All the functional JavaScript tests are failing with the same error:

1) Drupal\Tests\jsonapi_query_builder\FunctionalJavascript\JsonApiQueryBuilderJsTest::testReactAppLoads
Behat\Mink\Exception\ElementNotFoundException: Element matching css ".query-panel-tab[data-tab="fields"]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:465
/var/www/html/tests/src/FunctionalJavascript/JsonApiQueryBuilderJsTest.php:121

There are no more errors in the logs:

 ---- -------------- -------- ---------- ------------------------------------------------------------
  ID   Date           Type     Severity   Message
 ---- -------------- -------- ---------- ------------------------------------------------------------
  11   04/Sep 04:12   user     Info       User kw696290 used one-time login link at time 1756923133.
  10   04/Sep 04:12   user     Info       Session opened for kw696290.
  9    04/Sep 04:12   system   Info       jsonapi_query_builder module installed.
  8    04/Sep 04:12   system   Info       openapi_jsonapi module installed.
  7    04/Sep 04:12   system   Info       schemata_json_schema module installed.
  6    04/Sep 04:12   system   Info       schemata module installed.
  5    04/Sep 04:12   system   Info       openapi module installed.
  4    04/Sep 04:12   system   Info       jsonapi module installed.
  3    04/Sep 04:12   system   Info       serialization module installed.
  2    04/Sep 04:12   system   Info       field_ui module installed.
 ---- -------------- -------- ---------- ------------------------------------------------------------

Thank you, @ankitv18. Do you know when these issues were introduced? Are they already there on the main branch, or are they regressions from this issue? And if this issue has introduced them, did they work before my changes and now they're broken?

Are you following a test script on this issue, or are you doing unguided/exploratory testing? If there are tests you're repeating as the issue progresses, would you share your checklist?

I've identified and fixed several more secondary issues, including a 403 error due to a missing route. Here's where things stand as I take off for the weekend. (Monday is a federal holiday in the U.S., so I won't be back till Tuesday.)

I think all the functional JavaScript tests are failing for the same reason, so I'm focusing on fixing one of them first: \Drupal\Tests\jsonapi_query_builder\FunctionalJavascript\JsonApiQueryBuilderJsTest::testReactAppLoads(). The problem seems to be that the test browser (Mink/Selenium) doesn't render any React markup, while a real browser does. I've committed a few HTML dumps and screenshots for comparison:

• manual_test.htm (screenshot): Saved from a manual browser visit, shows the React app loading state as expected.
• custom-debugging.htm (screenshot): Captured by the test using $this->getSession()->getPage()->getOuterHtml(), but contains only CSS—no React app markup.
• simpletest_browser_output.html (screenshot): Generated by PHPUnit, also contains only CSS and no app markup.

All the functional JavaScript tests are failing with the same error:

1) Drupal\Tests\jsonapi_query_builder\FunctionalJavascript\JsonApiQueryBuilderJsTest::testReactAppLoads
Behat\Mink\Exception\ElementNotFoundException: Element matching css ".query-panel-tab[data-tab="fields"]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:465
/var/www/html/tests/src/FunctionalJavascript/JsonApiQueryBuilderJsTest.php:113

There are no more errors in the logs:

 ---- -------------- -------- ---------- ------------------------------------------------------------
  ID   Date           Type     Severity   Message
 ---- -------------- -------- ---------- ------------------------------------------------------------
  11   30/Aug 07:12   user     Info       User xlpztkl0 used one-time login link at time 1756501921.
  10   30/Aug 07:12   user     Info       Session opened for xlpztkl0.
  9    30/Aug 07:12   system   Info       jsonapi_query_builder module installed.
  8    30/Aug 07:12   system   Info       openapi_jsonapi module installed.
  7    30/Aug 07:12   system   Info       schemata_json_schema module installed.
  6    30/Aug 07:12   system   Info       schemata module installed.
  5    30/Aug 07:11   system   Info       openapi module installed.
  4    30/Aug 07:11   system   Info       jsonapi module installed.
  3    30/Aug 07:11   system   Info       serialization module installed.
  2    30/Aug 07:11   system   Info       field_ui module installed.
 ---- -------------- -------- ---------- ------------------------------------------------------------

That's what I know at the moment. If my Functional JavaScript Test fairy doesn't visit over the weekend, I'll pick this up again on Tuesday.

I've made a lot of progress here--I fixed the update hook a ton of static analysis, and some of the test issues--but I'm still having trouble with the React aspect of the JavaScript functional tests. I've improved the assertions a little and added a commit with some debugging output showing the HTML that Mink is getting before it starts looking for CSS selectors (as in, I actually committed the debugging output file so you can see it). It seems like the React app isn't initializing. It works just fine in manual testing, though. The failures are the same locally as on CI. Can anyone offer any guidance?

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

We don't say "please" in Drupal. 😄

https://www.drupal.org/docs/develop/user-interface-standards/interface-text#interface-text-style →

I'm certainly still interested. I still think it's one of the biggest potential improvements for content modeling in Drupal.

https://www.youtube.com/watch?v=fWKB8zdVM-U 😉

Confirmed and committed. Thank you, both!

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

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 → .