Skip to content

Conversation

@vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon commented Apr 24, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2759
License MIT
  • Check for parent ApiResource in RouteNameResolver
  • Update unit tests
  • Remove unfinished behat test features/graphql/non_resource.feature (false-positive in CI)
  • Add non-regression behat tests

@vincentchalamon vincentchalamon self-assigned this Apr 24, 2019
@vincentchalamon vincentchalamon force-pushed the issue/resource-inheritance branch 4 times, most recently from 4eeea0d to dd9d48a Compare April 24, 2019 09:25
@bendavies
Copy link
Contributor

bendavies commented Apr 24, 2019

I think maybe adding to https://github.com/api-platform/core/blob/master/features/main/table_inheritance.feature is appropriate here?

Edit: maybe not that feature, but at least a new functional test.

@vincentchalamon vincentchalamon changed the title Issue/resource inheritance Get resource from parent Apr 24, 2019
@vincentchalamon
Copy link
Contributor Author

@bendavies This PR is still in WIP, but yes I'll definitively add new behat tests 👍

@bendavies
Copy link
Contributor

@bendavies This PR is still in WIP, but yes I'll definitively add new behat tests 👍

hah ok :) all your checkboxes were done so i thought it was complete :)

@vincentchalamon
Copy link
Contributor Author

The main part of this PR is done, now it's time to add some non-regressions tests :)

@vincentchalamon
Copy link
Contributor Author

vincentchalamon commented Apr 24, 2019

I think maybe adding to https://github.com/api-platform/core/blob/master/features/main/table_inheritance.feature is appropriate here?

Edit: maybe not that feature, but at least a new functional test.

I think it should be the best place indeed. Thanks for it :)

@vincentchalamon vincentchalamon force-pushed the issue/resource-inheritance branch from f96cbc4 to ba95062 Compare April 24, 2019 10:19
@vincentchalamon vincentchalamon requested a review from soyuka April 24, 2019 10:20
@vincentchalamon vincentchalamon force-pushed the issue/resource-inheritance branch 3 times, most recently from ce6e679 to ab8de31 Compare April 24, 2019 12:00
Nek- added a commit to Nek-/core that referenced this pull request Apr 26, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request Apr 26, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request Apr 27, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760

@!mongodb
@createSchema
@dropSchema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't exists anymore IIRC

"pattern": "^/custom_users/\\d+$",
"required": "true"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
},
"additionalProperties": false

"pattern": "^/external_users/\\d+$",
"required": "true"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
},
"additionalProperties": false

$operation = $route->getDefault(sprintf('_api_%s_operation_name', $operationType));
$methods = $route->getMethods();

if (null !== $operation && (empty($methods) || \in_array('GET', $methods, true)) && null !== $currentResourceClass && is_a($resourceClass, $currentResourceClass, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking but maybe we could add this if statement in the previous loop and saved the first value found that would be returned at the after the loop if not null to avoid looping through all routes twice here ?

@vincentchalamon
Copy link
Contributor Author

Closing in favor of #2714

Nek- added a commit to Nek-/core that referenced this pull request Apr 29, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request May 2, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request May 2, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request May 2, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
Nek- added a commit to Nek-/core that referenced this pull request May 2, 2019
Previous commits fix the behavior with non api resources inherited from
resource but relation management was still not ok since the update was
not done on the abstract item normalizer.

The resource class resolver is now used as it should in every
normalizers.

Most of this commit (tests) comes from @vincentchalamon and its PR api-platform#2760
@teohhanhui
Copy link
Contributor

See #2797 instead.

@vincentchalamon vincentchalamon deleted the issue/resource-inheritance branch June 11, 2021 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants