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.

Sign up for GitHub

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

Jump to bottom

Lazy loaded Block Extension #676

Merged
merged 1 commit into from Mar 28, 2020
Merged

Lazy loaded Block Extension #676

merged 1 commit into from Mar 28, 2020

Conversation

mikemix
Copy link
Contributor

@mikemix mikemix commented Jan 29, 2020

Subject

I am targeting this branch, because it's a BC feature which makes the BlockExtension lazy.

Closes #675

Changelog

### Changed
* Lazy `BlockExtension` to speed Twig initialization

@mikemix
Copy link
Contributor Author

mikemix commented Jan 29, 2020

I'm not sure where's that error coming from ;)

mikemix
mikemix commented Jan 30, 2020
View reviewed changes
src/Resources/config/core.xml
@@ -34,6 +35,7 @@
<argument type="service" id="sonata.block.cache.handler.default" on-invalid="ignore"/>
<argument type="service" id="debug.stopwatch" on-invalid="ignore"/>
</service>
<service id="Sonata\BlockBundle\Templating\Helper\BlockHelper" alias="sonata.block.templating.helper"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required, because TwigFunction expects a callable to begin with.

core23
core23 previously approved these changes Jan 30, 2020
View reviewed changes
@VincentLanglet
Copy link
Member

HI @mikemix ! What's the status of this ? Do you have time to finish your PR ? :)

@mikemix
Copy link
Contributor Author

mikemix commented Mar 24, 2020 •
edited

It's ready since 30th of January. Test could be better, there's the FactoryRuntimeLoader one could use instead of implementing the RuntimeLoaderInterface by myself.

@VincentLanglet
Copy link
Member

There is the phpstan build failed, so this can't be merge.
Maybe a rebase can fix the issue...

@mikemix
Copy link
Contributor Author

mikemix commented Mar 24, 2020

I don't know what's causing this tbh. Event dispatcher error?

@VincentLanglet
Copy link
Member

I don't know what's causing this tbh. Event dispatcher error?

You need to rebase the 4.x branch because I think it was fixed. @mikemix

@mikemix
Copy link
Contributor Author

mikemix commented Mar 25, 2020

I'll do that. Also I'm going to fix the test. Late at night this evening probably.

@mikemix mikemix changed the title Lazy loaded Block Extension [WIP] Lazy loaded Block Extension Mar 28, 2020
@mikemix
Copy link
Contributor Author

mikemix commented Mar 28, 2020 •
edited

@VincentLanglet I don't like these tests ;) a real test using the IntegrationTestCase would be much better: https://twig.symfony.com/doc/2.x/advanced.html#testing-an-extension

Also, one unit test for the block_exists function was missing so I decided to add it, is it ok?

I'm done.

@mikemix mikemix changed the title [WIP] Lazy loaded Block Extension Lazy loaded Block Extension Mar 28, 2020
@greg0ire greg0ire added the minor label Mar 28, 2020
greg0ire
greg0ire approved these changes Mar 28, 2020
View reviewed changes
Copy link
Contributor

@greg0ire greg0ire left a comment •
edited

Choose a reason for hiding this comment

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

TIL about lazy extensions, nice feature! cc @laurent-bientz

@mikemix
Copy link
Contributor Author

mikemix commented Mar 28, 2020 •
edited

We will reduce tons of CO2 emission with this one I'm sure.

franmomu
franmomu approved these changes Mar 28, 2020
View reviewed changes
Copy link
Member

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

Nice, thank you for your first contribution!

@greg0ire greg0ire merged commit 025c25d into sonata-project:4.x Mar 28, 2020
@greg0ire
Copy link
Contributor

Thanks on fighting climate change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@franmomu franmomu franmomu approved these changes

@core23 core23 core23 left review comments

Assignees
No one assigned
Labels
minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make BlockExtension a lazy extension
5 participants
@mikemix @VincentLanglet @greg0ire @franmomu @core23