Problem/Motivation

Follow up #2922121: When fetching related resources, getResourceType() in CurrentContext gets the wrong resource type. to remove 

$this->markTestIncomplete('This fails and should be fixed by https://www.drupal.org/project/drupal/issues/2922121');

from \Drupal\Tests\jsonapi\Kernel\Normalizer\JsonApiDocumentTopLevelNormalizerTest::testNormalizeRelated.

In comment #8 we concluded this code is extra, and not needed so the test can be removed.

Steps to reproduce

Proposed resolution

Remaining tasks

Patch
Fix test
Review
Commit

User interface changes

API changes

Data model changes

Release notes snippet

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3213752-18.patch2 KB_pratik_
#2 3213752-2.patch836 bytesquietone

Issue fork drupal-3213752

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
3213752-2.patch836 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 3213752-2.patch, failed testing. View results

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Guessing this needs some actual work since the parent issue is closed but we cannot just enable this, that seems to fail since the current test doesn't really add include data from the author i think.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bradjones1 made their first commit to this issue’s fork.

bradjones1 opened merge request !2253

bradjones1’s picture

bradjones1
Primary language English
CreditAttribution: bradjones1 at Not Vanilla, Inc. commented
Status: Needs work » Needs review

So I ran into this skipped test while working on another issue and figured, what the heck, I'll see what's up.

For the life of me, I can't even really tell what this is intended to test that isn't already covered by ::testNormalizeUuid() (not a great name, for what it does...) and ::testNormalizeConfig().

This is very old code so I think this might boil down to, other tests take care of the intent here, and this can be removed?

I've made an MR to clean up this class overall and remove this test.

bradjones1’s picture

bradjones1
Primary language English
CreditAttribution: bradjones1 at Not Vanilla, Inc. commented
Title: Enable JsonApiDocumentTopLevelNormalizerTest::testNormalizeRelated » Remove dead code from JsonApiDocumentTopLevelNormalizerTest
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Needs review » Reviewed & tested by the community

The assertions are not really the same although it seems its only the check on getCacheMaxAge. But I agree the test does look like it is no longer needed.

Did notice a problem while looking at the code, made a new issue to remove some extra arguments that are not used. #3280302: JsonApiDocumentTopLevelNormalizerTest::generateProphecies called with extra arguments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

phpcs seems to be failing on the MR

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Huh. Did I miss that....?

bradjones1’s picture

bradjones1
Primary language English
CreditAttribution: bradjones1 at Not Vanilla, Inc. commented
Status: Needs work » Needs review

Funny, the errors have nothing to do with these changes but I guess this file hasn't been CS checked since perhaps rules tightened. Whatever, here it is.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Needs review » Reviewed & tested by the community

Thanks, looking good.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

Let's remove generateProphecies() altogether - it's not doing anything useful.

_pratik_’s picture

_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented
Assigned: Unassigned » _pratik_
_pratik_’s picture

_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3213752-18.patch2 KB

Please check if this works.

_pratik_’s picture

_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented
Assigned: _pratik_ » Unassigned

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
Issue tags: +Novice

Patch did not apply. Let's continue in the merge request?

ravi.shankar made their first commit to this issue’s fork.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented

Rebased the current MR as that was not mergeable, still needs work for reported threads on MR.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Hi, thanks for helping. But this patch needs to be moved to 10.1.x, you currently have rebased on the 9.4.x branch and i dont think it will be merged there.

rpayanm made their first commit to this issue’s fork.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Assigned: Unassigned » Spokje

Hiding patches, since we're in the MR workflow now.

Spokje opened merge request !3033

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Assigned: Spokje » Unassigned
Status: Needs work » Needs review

MR !3033 is against 10.1.x.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I just read the issue summary, got confused, read the comments again (#8) and it made sense again.

Thanks for adressing the feedback spokje. This is RTBC for me.

Note: updated IS so that doesnt happen again xD

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 4096338786 to 10.1.x and ebba427c49 to 10.0.x and 4c116a3f35 to 9.5.x. Thanks!

Backported to 9.5.x to keep test coverage aligned.

  • alexpott committed 4096338 on 10.1.x 
    Issue #3213752 by Spokje, bradjones1, quietone, _pratik_, ravi.shankar,...

  • alexpott committed ebba427 on 10.0.x 
    Issue #3213752 by Spokje, bradjones1, quietone, _pratik_, ravi.shankar,...

  • alexpott committed 4c116a3 on 9.5.x 
    Issue #3213752 by Spokje, bradjones1, quietone, _pratik_, ravi.shankar,...

Spokje closed merge request !3033

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Nice clean-up 😊 Thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.