Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

[WIP] Add block options #13

Closed
wants to merge 1 commit into from
Closed

Conversation

rmsint
Copy link
Contributor

@rmsint rmsint commented Jan 13, 2013

This PR adds the feature to overide options like the template. See https://groups.google.com/forum/?fromgroups=#!topic/symfony-cmf-devs/_t0jQCsdEcU I think this could be a solution, it works without BC breaks on the BlockInterface. It is probably far from good, but this will give an idea and could start discussion.

It allows to do things like this in the template:

    {{ sonata_block_render({
        'name': 'additionalInfoBlock',
        'template': 'SandboxMainBundle:Block:overloaded_container_block.html.twig',
        'attr': {
            'class': 'additional-info-block'
        },
        'settings': {
            'divisibleBy': 3,
            'divisibleClass': 'row-fluid',
            'childClass': 'span4'
        },
        'children': {
            'symfony_cmf.block.simple': {
                'template': 'SandboxMainBundle:Block:overloaded_block_simple.html.twig',
                'attr': {
                    'class': 'simple-block'
                }
            }
        }
    }) }}

Furthermore configuration for a block is split in:

  • settings: fe. a url for a feed, this configuration could be changed through a UI
  • options: configuration to be used in templates etc. but not changed through a UI

@lsmith77
Copy link
Member

the question is if this should be done inside SonataBlockBundle or in SymfonyCmfBlockBundle. @rande?

@rande
Copy link

rande commented Jan 13, 2013

I don't really have an opinion on this, a block must just return a response object, the way the block generates the response is the responsability of the block service.

@lsmith77
Copy link
Member

@rande: the question is if this should be a standard feature on the SonataBlockBundle base block or not

@rande
Copy link

rande commented Jan 13, 2013

I don't see the direct benefit of it.

@lsmith77
Copy link
Member

well this is a very general solution with the concrete use case of wanting to configure a template to be used in a block. seems like something that could be useful for many blocks?

@rande
Copy link

rande commented Jan 13, 2013

After an discussion with @lsmith77 on IRC, I will think about it. I think adding the OptionResolver is a good thing as it will solve ... settings resolutions. This will also deprecated the getDefaultSettings function.

@rmsint
Copy link
Contributor Author

rmsint commented Jan 14, 2013

ok cool, let me know if and how I can help to make changes

@lsmith77
Copy link
Member

@rmsint please ping me if there are any open PRs that are ready to be merged from your POV. keeping track of all if this is getting hard as there is too much stuff happening .. which is of course a good thing.

@dbu
Copy link
Member

dbu commented Jan 30, 2013

did something happen on the sonata block side by now?

@lsmith77
Copy link
Member

What is the main thing holding this PR back? is it sonata-project/SonataBlockBundle#43?

I think it would be very useful to be able to specify things like the template via the twig render call as well as via a property in the block instance. Would love to get this feature in ASAP.

@lsmith77
Copy link
Member

did you verify if this wouldn't cause issues with the cache layer?

@rmsint
Copy link
Contributor Author

rmsint commented Mar 17, 2013

@lsmith77 this PR is not depending on sonata-project/SonataBlockBundle#43. I think we wait for rande. didn't rande mention some time ago to pick this up after Symfony 2.2 release? Not sure because it is some time ago. I am also not sure of the exact impact on caching, however it will not do less than it does now. In #21 we concluded that it is best to save the settings in the block document to have it work properly when caching. So some of the new features this PR offers also will not work then.

@rande
Copy link

rande commented Mar 28, 2013

Here my proposal for symfony2.2, It a start sonata-project/SonataBlockBundle@d7b4138 but allows more flexibility.

Open for comments

@rmsint
Copy link
Contributor Author

rmsint commented Mar 28, 2013

@rande added some ideas in the code there, just skip them if not usefull..

@dantleech
Copy link
Member

I was just about to make a PR for this same feature (setting templates)

@dbu
Copy link
Member

dbu commented Apr 21, 2013

rande has a branch in sonata block bundle with the configuration. however, as he is on a long vacation, he did not want to merge a huge BC break right before he left. i fear we have to wait until mid-may until we can solve this topic.

@rmsint rmsint mentioned this pull request Apr 21, 2013
@rmsint
Copy link
Contributor Author

rmsint commented Apr 21, 2013

Probably this PR can be closed once the BlockContext is merged. We have to do some updates then, I tried to list them in this issue: #53

@dbu dbu mentioned this pull request Apr 23, 2013
@ghost ghost assigned rmsint Apr 25, 2013
@lsmith77
Copy link
Member

so this PR can be closed?

@rmsint
Copy link
Contributor Author

rmsint commented May 24, 2013

yes, I think so. Only thing this PR has additional is that it also allows to pass configuration for children blocks that could be usefull when using the container block. Will ask during irc meeting and then create a ticket if needed.

@rmsint rmsint closed this May 24, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants