symfony-cmf / block-bundle

Extends the SonataBlockBundle to integrate with PHPCR ODM
https://cmf.symfony.com
20 stars 51 forks source link

multiple embed blocks fix #252

Closed vcraescu closed 8 years ago

vcraescu commented 8 years ago

Embed multiple blocks it's not working when you use the default prefix and postfix: %embed-block|, |end%. Also, preg_* functions should be avoided parsing large files due their length limit.

dbu commented 8 years ago

awesome, great work! i just have a few nitpiks that i commented on, otherwise looks good.

vcraescu commented 8 years ago

Hi David!

I'll do another PR during the weekend with this fix which will not break 1.x interface. I'm already using this feature into a Sylius project so it might take some time till they move to 2.0 :)

Thanks!

dbu commented 8 years ago

for 2.0 we perfer to do the BC break to clean things up, so this code looks fine. the BC solution for 1.x can be to simply instantiate the parser within the helper instead of using DI, right?

though, for the sake of the git history and keeping master in sync with the 1.x branch, i think its best if you cherry-pick this commit into a branch off the 1 branch, do that PR and once we merge this, i update master from 1. then you can rebase this PR and it will not add the whole parser but just fix the setup and service. does that make sense?

vcraescu commented 8 years ago

I was thinking to move the the entire parse code directly into the helper for 1.x version. Version 2 will be just a refact of this code where parse gets its own class. If we instantiate the parser with the helper then in version 2 we will have 2 compatibility breaks. Do you agree?

dbu commented 8 years ago

sounds like a good plan to me. so i wait with merging this until you ported it to the 1.3 branch (we don't maintain the older branches anymore)

vcraescu commented 8 years ago

Sure. I'll do it this weekend.

dbu commented 8 years ago

next step: #255 and then rebase this branch on master.

dbu commented 8 years ago

253 is now in master. please rebase this PR and fix the conflicts, then we can merge this.

vcraescu commented 8 years ago

I will do it this weekend. Sorry I had a crazy week and didn't get a chance to take care of this. :(

vcraescu commented 8 years ago

Can you help me out with these build errors?

dbu commented 8 years ago

hm, that error seems unrelated to your changes. we also see it in master. probably changes in one of our also-not-released dependencies that prevent installing the bundle. i will have a look.

dbu commented 8 years ago

okay, fixed the build in #256. can you please rebase this branch on master? it should then be green again.

vcraescu commented 8 years ago

Why didn't you merge this into master? Any issues?

dbu commented 8 years ago

thanks! sorry, did not see the notification for your push.

vcraescu commented 8 years ago

Np. Thanks! :)

dbu commented 8 years ago

thank you for the contribution!

vcraescu commented 8 years ago

uw!