Skip to content
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

Deprecate usage of ob_* functions in favor of yielding #3950

Merged
merged 12 commits into from
Feb 5, 2024

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented Dec 22, 2023

No description provided.

@fabpot fabpot force-pushed the yielding branch 18 times, most recently from d7691f7 to f4201ff Compare December 25, 2023 10:29
fabpot added a commit that referenced this pull request Dec 30, 2023
This PR was merged into the 3.x branch.

Discussion
----------

Abstract node capture in its own Node

To prepare the work for #3950, I'd like to abstract the usage of `echo` into a single node that can be reused by other nodes.

The cache node is a good example of defining something that is more semantic are reusable as well. The node itself now just generates the wrapper that can then be returned, echoed, yielded, ...

Commits
-------

6ebbc30 Abstract node capture in its own Node
@xabbuh
Copy link
Contributor

xabbuh commented Jan 2, 2024

FYI, I have run Symfony tests against this branch and did not spot any failures.

src/Node/YieldTextNode.php Outdated Show resolved Hide resolved
src/Node/YieldExpressionNode.php Outdated Show resolved Hide resolved
src/Node/ModuleNode.php Show resolved Hide resolved
src/Environment.php Show resolved Hide resolved
src/YieldingTemplate.php Show resolved Hide resolved
@smnandre
Copy link
Contributor

smnandre commented Jan 5, 2024

(all green with TwigComponent / LiveComponent)

@fabpot fabpot force-pushed the yielding branch 2 times, most recently from 69f20a6 to e44c77d Compare January 5, 2024 18:18
@fabpot
Copy link
Contributor Author

fabpot commented Jan 5, 2024

(all green with TwigComponent / LiveComponent)

Thank you for testing!

@ghost
Copy link

ghost commented Jan 21, 2024

Hi @fabpot! Is it possible to update PR description with the motivation and pros of this change? ob_* functions and yielding are niche enough (the former especially) that it would help to understand this change for not so advanced developers.
Thank you in advance.

@stof
Copy link
Member

stof commented Jan 24, 2024

@javaDeveloperKid the usage of output buffers makes Twig incompatible with code using Fibers for instance, because suspending a fiber to resume another one does not change the active output buffer.

@ghost
Copy link

ghost commented Jan 24, 2024

@javaDeveloperKid the usage of output buffers makes Twig incompatible with code using Fibers for instance, because suspending a fiber to resume another one does not change the active output buffer.

@stof Thank you. I really appreciate you are always willing to answer questions and share your knowledge. Even questions that are not addressed to you.
So refactor to yielding here is not about memory consumption or even modernizing the codebase?

@stof
Copy link
Member

stof commented Jan 24, 2024

@javaDeveloperKid intuitively, both architectures are probably using a similar amount of memory: we still need memory holding the whole generated output.
Regarding modernizing the code base, this alone would not be a justification for a BC-breaking rewrite of the architecture, if the new one were not providing extra benefit (remember that we are talking about the generated code here mostly).

doc/advanced.rst Outdated Show resolved Hide resolved
src/Node/BlockNode.php Outdated Show resolved Hide resolved
src/Node/Expression/InlinePrint.php Show resolved Hide resolved
src/Node/ModuleNode.php Outdated Show resolved Hide resolved
@fabpot
Copy link
Contributor Author

fabpot commented Jan 25, 2024

@javaDeveloperKid intuitively, both architectures are probably using a similar amount of memory: we still need memory holding the whole generated output. Regarding modernizing the code base, this alone would not be a justification for a BC-breaking rewrite of the architecture, if the new one were not providing extra benefit (remember that we are talking about the generated code here mostly).

Being able to use fibers is the main argument.

I would add that using ob_* functions always seemed awkward and hacky. The code reads better with yields and seems more idiomatic.

As we don't release major releases that often, that's always a good occasion to modernize the code.

@GromNaN
Copy link
Contributor

GromNaN commented Jan 30, 2024

This change allows to use asynchronous functions in twig templates. I built a small demo app with ReactPHP and Twig: https://gist.github.com/GromNaN/37e661f2ff06735d8b93ff93b24f8257

@ghost
Copy link

ghost commented Feb 2, 2024

This change allows to use asynchronous functions in twig templates. I built a small demo app with ReactPHP and Twig: https://gist.github.com/GromNaN/37e661f2ff06735d8b93ff93b24f8257

@GromNaN Ok, so it's a framework designed for asynchronous PHP coding that can leverage changes from this PR. I thought that Twig itself will have some parts working asynchronously somehow.

@fabpot fabpot merged commit edf5808 into twigphp:3.x Feb 5, 2024
2 of 3 checks passed
@fabpot fabpot deleted the yielding branch February 5, 2024 13:24
weaverryan added a commit to symfony/ux that referenced this pull request Feb 14, 2024
…rue` for now) (smnandre)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Twig][Live] Skip Twig 3.9 🚒  (do not set `use_yield = true` for now)

As Symfony Twig Bridge & Twig Bundle have decided to skip Twig 3.9 for now, we should do the same, as testing and/or maintaining a compatible version would be really hard without the bundle support.

symfony/twig-bridge@2abddb1
symfony/twig-bundle@23a02ff

Update:

Twig 3.9 introduce a major change in the way templates are rendered, and to fully test/fix our compatibility, we need to wait Bridge and Bundle to allow Twig 3.9

In the meantime, please do not set "use_yield = true" as we are not ready yet.

This is something that has a major impact on TwigComponent and LiveComponent, and we'll probably need some changes on the CI / test suite to handle both modes.

---

Pull Request (compatibility - wip) : #1487 (help welcomed)
Issue: #1390
Twig PR : twigphp/Twig#3950

Commits
-------

6165384 [Twig][Live] Skip Twig 3.9 🚒  (do not set `use_yield = true` for now)
symfony-splitter pushed a commit to symfony/ux-live-component that referenced this pull request Feb 14, 2024
…rue` for now) (smnandre)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Twig][Live] Skip Twig 3.9 🚒  (do not set `use_yield = true` for now)

As Symfony Twig Bridge & Twig Bundle have decided to skip Twig 3.9 for now, we should do the same, as testing and/or maintaining a compatible version would be really hard without the bundle support.

symfony/twig-bridge@2abddb1
symfony/twig-bundle@23a02ff

Update:

Twig 3.9 introduce a major change in the way templates are rendered, and to fully test/fix our compatibility, we need to wait Bridge and Bundle to allow Twig 3.9

In the meantime, please do not set "use_yield = true" as we are not ready yet.

This is something that has a major impact on TwigComponent and LiveComponent, and we'll probably need some changes on the CI / test suite to handle both modes.

---

Pull Request (compatibility - wip) : symfony/ux#1487 (help welcomed)
Issue: symfony/ux#1390
Twig PR : twigphp/Twig#3950

Commits
-------

61653842 [Twig][Live] Skip Twig 3.9 🚒  (do not set `use_yield = true` for now)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants