- First commit to issue fork.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
- I think the fix should be different: the UI should inform the user that the requested component instance longer exists. While the current MR may subdue the symptoms, it does so by using the ostrich technique π
- Why isn't the OpenAPI request validation not detecting this? π€ Ah β¦ because
/openapi.yml
is incomplete! AFAICT this addition should surface the problem right away:
openapi.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/openapi.yml b/openapi.yml index 6b5c2af8..bfba2303 100644 --- a/openapi.yml +++ b/openapi.yml @@ -149,6 +149,12 @@ paths: parameters: - $ref: '#/components/parameters/entityTypeId' - $ref: '#/components/parameters/entityId' + # A JSON blob representing the edited component subtree (a single node, really). + - in: query + name: tree + required: true + schema: + type: string get: responses: '200':
π Additions like that are why I asked π Add missing API paths + request bodies to `openapi.yml` Active to be opened for.
- π¬π§United Kingdom longwave UK
When this happens
tree
is the stringnull
. The culprit is DummyPropsEditForm:const tree = findNodeInObjectTree(layout, selectedComponent); const query = new URLSearchParams({ tree: JSON.stringify(tree), props: JSON.stringify(preparedModel), selected: selectedComponent, latestUndoRedoActionId, }); setDynamicStaticCardQueryString(`?${query.toString()}`);
findNodeInObjectTree
can be null if we didn't find the selected component; on page load this appears to happen sometimes while things are still being loaded into place; the actual tree (layout
) has no children at this point. In manual testing there always seemed to be a valid request immediately afterwards, so we maybe can just skip the case wheretree
is null and not trigger the query?Unsure if or how to write a test for this.
- @longwave opened merge request.
- First commit to issue fork.
- πΊπΈUnited States effulgentsia
Is the warning happening on this line?
$component_machine_name = json_decode($this->getRequest()->get('tree'), TRUE)['type'];
If so, what is the value of
$this->getRequest()->get('tree')
when the warning happens? Automatically closed - issue fixed for 2 weeks with no activity.
- πΊπΈUnited States TravisCarden
FYI: I'm setting this issue aside for the time being in favor of higher priorities.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
On the right track π β¦ but it's too PHP-heavy. This test coverage must have a very low bar for non-PHP developers. See MR with suggestions.
- πΊπΈUnited States TravisCarden
tl;dr: I agree with everything you said, and I have an "inexpensive" solution I would like to implement that I would consider to satisfy the requirements of this issue.
IMHO functional tests (
BrowserTestBase
+ Cypress E2E tests) is sufficient for now β that's sufficient for testing the common paths.I agree.
The goal for OpenAPI is to help the front and back ends to stay in sync with more ease, not for that to become a goal unto itself.
Yes. We're on the same page here.
That being said, if you see a low-effort/maintenance, high-impact/reach way to write kernel or unit tests to verify certain edge cases in request/response bodies are caught automatically by OpenAPI, then I'd definitely welcome that :)
I have proposed one in the above MR. I actually hadn't planned on looking at this issue at all yet, but working on π Add missing API paths + request bodies to `openapi.yml` Active I was having a hard time testing my assumptions about how the specification itself was being interpreted. And the ability to just whip up a quick PHP array and get instant feedback really clarifies things and saves a lot of "throwing things at the wall" and "testing by coincidence". (Turns out even YAML can be written test-first! π)
But that should not be a multi-week/month endeavor IMHO.
It just took a day. π
Re: the MR
The tests are passing, but I notice PHPUnit reports two tests skipped--which I assume are the two that this MR adds. That is doubtless because the tests depend on
league/openapi-psr7-validator
, which I assume isn't installed on CI. Ordinarily that would constitute a problem that needs solving, of course, but I'm actually fine with it in this case, since its whole point is to aid in rapid development and the real "regression tests" are in Cypress. Developers can just run them ad hoc in PhpStorm, for example (as I've been doing), when actually modifyingopenapi.yml
and ignore them the rest of the time. They could technically get out of sync, but as far as I'm concerned, we can create a follow-up to worry about that later.So I would like to propose the MR above as a rapid development tool that satisfies our minimal requirements and lays a foundation of future iteration if that ever seems valuable. According to your judgment, we can create a follow-up issue and/or a
@todo
or a note in theREADME
I added to capture the above issue of the tests not getting run on CI. - @traviscarden opened merge request.