🇺🇸United States @traviscarden

Hey, @amateescu. I'm afraid I no longer have allocation for this as I've been moved to another project. I'll be cheering from the sidelines. 😄

Nice, thanks, @alexpott!

I don't know what the whole merge train comment there is about, but I did merge the MR, and the commit is in 2.1.x.

Would you like a release?

Fix an ironic little typo in the issue title and... voilà.

traviscarden → created an issue.

traviscarden → created an issue.

When you say the authentication "skews what users will see when making requests", @mglaman, do you mean that, as an authenticated user, you could potentially see results that unauthenticated requests to the public API endpoint would not see? Put simply, you won't necessarily see the same things in the UI what your target audience would see in real life?

That makes perfect sense to me, and I agree that not authenticating requests is the best and safest default behavior. Do you think that behavior should be configurable, maybe via a checkbox in the UI?

@balsama, do you think we should add this to our CI job?

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

To correspond to its menu position, it should be at /admin/config/services/jsonapi-query-builder. The JSON:API settings page adjacent to it in the menu is at /admin/config/services/jsonapi, which makes it a doubly safe choice, in my opinion.

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

Thanks, @balsama. The fix for that issue* just hadn't been committed yet at the time this MR was created. I've rebased. It should work and the problem should be gone now.

* 🐛 Errors when sorting Active

Ya know what? As long as we're touching this code, let's make one more related enhancement and disable "Sort" fields once added to prevent adding the same one more than once.

traviscarden → created an issue.

traviscarden → created an issue.

traviscarden → created an issue.

The problem in both cases was that the "Sort" dropdown was showing invalid options, i.e., options that shouldn't have been in the list to begin with because the endpoint doesn't actually support them. This MR prevents that from happening anymore.

traviscarden → created an issue.

@ankitv18 I have confirmed that the bug(s) persist on 2.0.x-dev. And there are actually quite a few fields in addition to the ones you listed that cause the same or similar errors. We'll have to investigate whether they all have a common cause or not.

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.