- Issue created by @bbrala
- 🇺🇸United States bradjones1 Digital Nomad Life
At first glance I like option 2 more, since it's also reducing often-repeated boilerplate.
- 🇳🇱Netherlands bbrala Netherlands
Updated issue summary, changed default for second option to TRUE since most calls require a data key in the reponse. Sometimes you want to check errors, this would then mean setting the second argument to FALSE (or not using that method ;))
- Merge request !7627Issue #3439440 by nicxvan, Binoli Lalani, longwave: Remove country support from DateFormatter → (Closed) created by bbrala
- Merge request !7628Add new GetDocumentFromRequestTrait to simplify getting document from response... → (Open) created by bbrala
- 🇳🇱Netherlands bbrala Netherlands
Ok, tests are green. While moving the tests had a nice moment when I already got some better feedback ^^
1) Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testWrite Missing expected data property in document. Errors: Bad Request: Resource object must include a "type".
I'll open a MR also with some test failures.
- Merge request !7637Draft: Add new GetDocumentFromRequestTrait to simplify getting document from response... → (Closed) created by bbrala
- Status changed to Needs review
8 months ago 11:14am 21 April 2024 - 🇳🇱Netherlands bbrala Netherlands
Example failure when passing in invalid langcode.
Drupal\Tests\jsonapi\Functional\JsonApiFunctionalMultilingualTest::testPatchTranslation Missing expected data property in document. Error(s): Unprocessable Content: langcode.0: The value you selected is not a valid choice. core/modules/jsonapi/tests/src/Traits/GetDocumentFromRequestTrait.php:33 core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalMultilingualTest.php:173 vendor/phpunit/phpunit/src/Framework/TestResult.php:729
- 🇳🇱Netherlands bbrala Netherlands
The testfailure is an unrelated failure in
Drupal.FunctionalJavascriptTests.Ajax.AjaxTest
. - 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3440993-jsonapi-test-failures to hidden.
- 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3440993-jsonapi-test-failures to active.
- 🇺🇸United States bradjones1 Digital Nomad Life
A few nits and renamings, but I think this is a nice DX and test ergonomics improvement, and cuts down on code repetition. More maintainable, too.
- 🇳🇱Netherlands bbrala Netherlands
Would be nice if we can RTBC this, got a committers eye on this, which could mean this could get in pretty quickly. Would love to use this feature soon to make development easier.
- Status changed to RTBC
8 months ago 2:33pm 22 April 2024 - 🇺🇸United States bradjones1 Digital Nomad Life
While I made a commit on this it was basically also a code-review so I will be bold and RTBC this.
- Status changed to Needs work
8 months ago 4:15am 23 April 2024 - 🇳🇿New Zealand quietone
It took a while to look at the changes but I got there in the end. I left some questions in the MR.
- Status changed to Needs review
8 months ago 6:40am 23 April 2024 - 🇳🇱Netherlands bbrala Netherlands
Good suggestions. I've changed the signature and updated the weird variable name.
In regards to the order of 200 vs getting the document, in my opinion it is important to change the order of those to get optimal value from this change.
- Status changed to RTBC
8 months ago 8:43am 23 April 2024 - 🇳🇿New Zealand quietone
Thanks for pointing that out. It makes perfect sense to do the status check after.
The items I pointed out have been addressed, so restoring RTBC
- 🇦🇺Australia acbramley
This is great! Anything to improve these tests is much appreciated, maybe we should turn 📌 Tests extending jsonapi's ResourceTestBase are extremely hard to debug/maintain/deal with Active into a meta?
Does this not need to go into 11.x first? mr is against 10.3.x
- Status changed to Needs work
8 months ago 3:27pm 26 April 2024 - 🇺🇸United States bradjones1 Digital Nomad Life
Re: #30 good catch, the MR applies clean against 11.x but it does need a target change. @bbrala, since you own that MR?
NW briefly for that, and to re-queue tests against HEAD.
- 🇳🇱Netherlands bbrala Netherlands
Huh, there was a second one? Might be mixing up two issues though. I'll fix it
- Status changed to Needs review
8 months ago 4:51pm 26 April 2024 - 🇳🇱Netherlands bbrala Netherlands
Screwed up a rebase lol ;p
Anyways, all should be fine again.
- Status changed to RTBC
8 months ago 5:25pm 26 April 2024 - 🇺🇸United States bradjones1 Digital Nomad Life
Rebase looks good, target changed. Back to RTBC.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
This feels like a quality of life task to me.
- Status changed to Needs work
8 months ago 10:57pm 29 April 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Just one comment on the MR, fine to self RTBC
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Updated issue summary to remove the redundant option 1.
Can we get a change record here please? Thanks!
- Status changed to RTBC
8 months ago 7:20am 30 April 2024 - 🇳🇱Netherlands bbrala Netherlands
Moved interface up, and added a change record → describing the improvement.
- 🇳🇱Netherlands bbrala Netherlands
Hmmz, I think this needs a last minor change.
$errors[] = $error['title'] . ': ' . $error['detail'];
Should be:
$errors[] = $error['title'] . '(' . $error['status'] . '): ' . $error['detail'];
That will help in some cases.
- 🇳🇱Netherlands bbrala Netherlands
I'm gonna be bold and keep rtbc. Seems like a really really minor change.
-
quietone →
committed 0fe7080e on 10.3.x
Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...
-
quietone →
committed 0fe7080e on 10.3.x
-
quietone →
committed f2fb38ac on 10.4.x
Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...
-
quietone →
committed f2fb38ac on 10.4.x
-
quietone →
committed 74a6dce2 on 11.x
Issue #3440993 by bbrala, bradjones1, larowlan, quietone: Improve JSON:...
-
quietone →
committed 74a6dce2 on 11.x
- Status changed to Fixed
8 months ago 4:11am 3 May 2024 - 🇳🇿New Zealand quietone
I was about to commit this when I realized that there are no tests of the new trait. I then checked with the heuristics → in the Testing Core Gate, Gong through the questions I concluded that this does not need tests. And I did force errors locally to confirm that it does indeed produce the improved messages. Still, I checked in committer Slack.
After chatting with larowlan we both agree that tests are not needed here. He also pointed out that manual testing was also done in #11, which I had forgotten. he would like to backport to 10.2.x and I don't think this meets the criteria in the 'allowed changes' doc.
Therefore, committed to 11.x, 10.4.x and 10.3.x
- 🇳🇱Netherlands bbrala Netherlands
Awesome, this will help a lot while developing jsonapi. :)