Skip to content

fix: Resolve nested relations #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

silasjoisten
Copy link
Collaborator

@silasjoisten silasjoisten commented Jul 25, 2025

closes #63 #62

@silasjoisten silasjoisten self-assigned this Jul 25, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2025

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.79%. Comparing base (006cdab) to head (6729710).

Files with missing lines Patch % Lines
src/Resolver/StoryResolver.php 88.23% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #66      +/-   ##
============================================
- Coverage     93.88%   93.79%   -0.09%     
- Complexity      461      464       +3     
============================================
  Files            72       72              
  Lines          1620     1629       +9     
============================================
+ Hits           1521     1528       +7     
- Misses           99      101       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

foreach ($target as &$value) {
if (\is_string($value) && \array_key_exists($value, $relationMap)) {
$value = $relationMap[$value];
return $this->doResolve($target, $relationMap, []);
Copy link

@frastel frastel Jul 25, 2025

Choose a reason for hiding this comment

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

Here are two things done:

  • deep resolving of relations
  • not to replace the uuids themselves

This would fix my both opened issues. But I think you could simplify this approach a bit.
We have implemented our own StoryResolver in the meantime (I had not enough time yet to create a PR with our version) and we did it like this:

// resolve relations which may be included in the resolved relations themselves
$this->doResolve($relationMap, $relationMap);

// resolve relations in the main target
$this->doResolve($target, $relationMap);

return $target;
        
private function doResolve(array &$target, array $relationMap): void
//...
// uuid check here like it is done in this PR

We are passing the $target as reference because our API responses are quite huge and we want to lower the memory usage on our servers.

In this case you do not need the $seen parameter and can assume that the resolved relations are already resolved when the main stories are looped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolved Relations are not resolved themselves
3 participants