Skip to content

Conversation

@mtarld
Copy link
Contributor

@mtarld mtarld commented Oct 14, 2025

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #62030
License MIT

Having outdated stream PHP files can lead to unexpected results.

Therefore, this PR leverages the use of ConfigCache to regenerate streamers on resource (ie: class) change.

@MatTheCat
Copy link
Contributor

Thanks for looking into this! Unfortunately this didn’t solve the issue, maybe because the target object is not necessarily a resource?

@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch from b5fee7c to 32fc3c6 Compare October 14, 2025 09:53
@mtarld
Copy link
Contributor Author

mtarld commented Oct 14, 2025

Indeed, I registered objects as ReflectionClassResource so that objects are tracked no matter where they come from, let me know if it solves the issue 🙂

@mtarld
Copy link
Contributor Author

mtarld commented Oct 14, 2025

But to work, the object needs, of course, to be marked with #[JsonStreamable] (which is what's written in the documentation).

@MatTheCat
Copy link
Contributor

MatTheCat commented Oct 14, 2025

The documentation says it’s optional?

I’m still not seeing any difference though.

@stof
Copy link
Member

stof commented Oct 14, 2025

see my comment on the issue. Making the cache warmer required and invalidating the whole container cache when the streamable class gets edited is overkill IMO.
In dev, we should rather keep generating the streamable classes on demand (there will be tons of cases where you will edit something in your project without running code for all your streamable classes, where a required class warmer would slow down your dev env for nothing), but with proper cache invalidation.

@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch from 32fc3c6 to 6a4470c Compare October 14, 2025 14:46
@mtarld mtarld changed the title [JsonStreamer] Make cache warmers non-optional [JsonStreamer] Rebuild cache on class update Oct 14, 2025
@mtarld
Copy link
Contributor Author

mtarld commented Oct 14, 2025

@stof, the PR has been updated to leverage config cache factories (like in translator and router).

Let me know if it's the right approach (I'm not used to that).

@MatTheCat
Copy link
Contributor

This is better but it seems the cache won’t be refreshed if a nested class changes.

@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch 4 times, most recently from eee479e to 348d4e5 Compare October 21, 2025 08:27
@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch from 348d4e5 to 84c17a5 Compare October 25, 2025 07:35
@mtarld
Copy link
Contributor Author

mtarld commented Oct 25, 2025

Low deps tests should be fixed once merged.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 25, 2025

Low deps tests should be fixed once merged.

let me doubt about this: merging up won't change anything to lowest deps

@mtarld
Copy link
Contributor Author

mtarld commented Oct 25, 2025

My thought was:

  • FrameworkBundle is failing with a too low version on JsonStreamer (<=7.3.4 which is without config cache)
  • If FrameworkBundle is requiring JsonStreamer at the next version (^7.3.5), it should work again

Thinking to this again, I'm indeed wrong, because FrameworkBundle:^7.3.5 can still require JsonStreamer:^7.3.2 for example. Therefore:

  • maybe we need to add something like a conflict to symfony/json-streamer: <7.3.5?
  • or maybe I update the test with a class_exists on StreamerDumper in the test to make it dependent of that fix?

WDYT?

@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch 2 times, most recently from 63a9a14 to 05628a9 Compare October 29, 2025 07:44
@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch from 05628a9 to e8c5a10 Compare November 12, 2025 16:44
@mtarld mtarld force-pushed the fix/json-streamer-virtual-property branch from e8c5a10 to cd32dac Compare November 12, 2025 16:46
@nicolas-grekas
Copy link
Member

Thank you @mtarld.

@nicolas-grekas nicolas-grekas merged commit 4961eea into symfony:7.3 Nov 12, 2025
10 of 11 checks passed
@mtarld mtarld deleted the fix/json-streamer-virtual-property branch November 12, 2025 17:07
mtarld added a commit to mtarld/symfony that referenced this pull request Nov 13, 2025
nicolas-grekas added a commit that referenced this pull request Nov 13, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

[JsonStreamer] Finish #62063 upmerge

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | F
| License       | MIT

Finish the ongoing #62063 upmerge to 7.4.

/cc `@nicolas`-grekas

Commits
-------

c22d2d8 [JsonStreamer] Finish #62063 upmerge
nicolas-grekas added a commit that referenced this pull request Nov 13, 2025
* 7.4:
  fix merge
  [HttpFoundation] Fix tests
  [JsonStreamer] Finish #62063 upmerge
  [Console] Fix signal handlers not being cleared after command termination
  [HttpFoundation] Fix RequestTest insulation
  ReflectionMethod::setAccessible() is no-op since PHP 8.1
  CS fix
  fix merge
nicolas-grekas added a commit that referenced this pull request Nov 13, 2025
…rekas)

This PR was merged into the 7.3 branch.

Discussion
----------

[FrameworkBundle] Fix wiring JsonStreamReader

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Providing FC/BC to #62063

Commits
-------

215caa3 [FrameworkBundle] Fix wiring JsonStreamReader
This was referenced Nov 13, 2025
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