[RFC] Proposal for new configuration format. #38

dantleech · 2013-11-15T21:10:49Z

No description provided.

dbu · 2013-11-17T01:00:44Z

Tests/Resources/app/config/routingautoroute.yml

-                        strategy: use
-                    not_exists_action:
-                        strategy: create
+                    provider: [ specified, { path: test/auto-route } ]


what is specified doing in this case? acting as a name for this? or is it a "type" of provider? if its defining the name, i prefer the previous format. if its defining the "type", i like this new compact format better.

It is the name of the strategy to use.

Another (complimentary or alternative) syntax would be to use an associative array:

provider: { strategy: specified, options: { path: test/auto_route } }

i like the succintctness of the short variant. but the other one looks good too. maybe more flexible in case we add additional things in some places. @wouterj wdyt?

dbu · 2013-11-17T01:01:54Z

i am +1 on this, looks really nice and more compact. and quite expressive.

wouterj · 2013-11-17T14:34:42Z

Tests/Resources/app/config/routingautoroute.yml

-        ##
+                    - auto_increment
+                    - 
+                        pattern: -%d


this is the same as [ auto_increment, { pattern: -%d } ] isn't it? Why did you use this (ugly) format?

This is just for getting an idea of what the configuration can look like when expanded.

wouterj · 2013-11-18T13:54:54Z

ping @dantleech

I've been working on this one. I've added tests for the config and implemented the config format. Currently many things are supported:

# long version
provider:
    name: content_method
    options:
        method: getTitle

# short version
provider: [ content_method, { method: getTitle } ]

# long version without options
exists_action:
    strategy: use

# short version without options 1
exists_action: [ use ]

# short version without options 2
exists_action: use

I've also removed the duplicated lines in the config tree and replaced them with the appending feature.

wouterj · 2013-11-18T13:58:48Z

Are we also going to fix the XML issues with the current configuration? If so, there are currently 2 things to discuss:

The auto_route_mapping node needs a fixXmlConfig in order to work. This means we should rename it to auto_route_mappings (plural version), what do you think about this?
Using multiple content-path elements is not nice, do you agree with using builder-unit elements in XML? Should we also do this in Yaml, or do we want to use some beforeNormalization to not include this in the Yaml and PHP format?

dbu · 2013-11-18T14:11:11Z

looking good, although we should not have too many different options for the same thing. one short and one long should be enough imho. for xml, i would vote to keep them as much as possible in sync. auto_route_mappings sounds semantically more correct to me.

dantleech · 2013-11-18T14:15:58Z

That looks perfect. For auto_route_mappings I think changing it rather to mapping(s) is better. For plurality - what do the other bundles do?

builder-unit does not really say much -- its more a term used in the code.

<path_unit ...>
<path_name ...>

What would that look like?

wouterj · 2013-11-18T14:28:10Z

For plurality - what do the other bundles do?

It's just required and almost all bundles use it, afaik.

What would that look like?

I think I prefer <path-unit>.

although we should not have too many different options for the same thing. one short and one long should be enough imh

Almost all versions are comming from the config component. The long version is required and when not providing options it just looks like the other long version. The short version is too nice to not include it and using it without any options looks a bit ugly to me, that's why I introduced only providing the name.

I don't think it'll cause an issue, we just advocated using the short version and nobody will use the long version.

wouterj · 2013-11-18T14:56:42Z

OK, I've now finished this PR with tests for PHP and XML support. Wdyt?

dbu · 2013-11-18T15:33:14Z

+1 , great stuff!

it needs a merge of master branch and resolve conflicts it seems.

now we "just" need to document this gem and make it follow the bundle standards to have it ready for the december release of the cmf :-)

dantleech · 2013-11-18T15:56:04Z

Yeah looks good :) For this to be RC I think it needs #33 and #37 -- the multilang depends on that PHPCR-ODM issue with translation loading from memory ...

wouterj · 2013-11-18T16:00:21Z

rebased it

I can't see why we need to mere #33 before merging this one? And for #37, I think it's better to merge this one first and then update that PR.

dantleech · 2013-11-18T16:05:42Z

Sorry, not as a prerequisite for merging, but for a 1.0 version.

Although, it might be a good idea to make a tag here as its a BC break.

dbu · 2013-11-18T16:54:42Z

a tag like 0.9 ? would make sense for people depending on this.

dbu · 2013-11-18T16:55:20Z

another option could be to call it 1.0 but with a big warning that BC is going to break. would be a good excuse to have the real thing be version 1.1 as the rest of the cmf...

dantleech · 2013-11-18T21:17:41Z

The tests seem to be broken...

dantleech · 2013-11-18T21:19:04Z

DependencyInjection/Configuration.php

+                                        ->useAttributeAsKey('name')
+                                        ->prototype('array')
+                                            ->children()
+                                                ->append($this->getUnitConfigOption('provider', 'name'))


@wouterj I changed the name of this method to BuilderUnitConfigOption to UnitConfigOption .. slightly more generic.

wouterj · 2013-11-18T21:37:46Z

@dantleech tests should pass now

dantleech · 2013-11-18T22:27:09Z

Tests/Resources/Fixtures/config/config.yml

+    mappings:
+        Acme\BasicCmsBundle\Document\Page:
+            content_path:
+                path_units:


Not sure I like this key, it is redundant. The content_path should already be a collection of "path-units". It also suggests that content_name should have a path_unit child.

content_path: pages: { provider: [ specified, { path: ... } ] } foobar: { provider: [ specified, { path: ... } ] }

I made the path_units child optional.

dantleech · 2013-11-23T06:47:57Z

There is a single test fail about "path_units" -- otherwise is this ready to go?

wouterj · 2013-11-24T15:49:43Z

@dantleech it should be fixed now

dantleech · 2013-11-24T16:05:54Z

@wouterj could you update the documentation with the new format? I will then tag what we currently have as 0.9 or something and then merge this :)

wouterj · 2013-11-24T18:42:22Z

see: symfony-cmf/symfony-cmf-docs#338

wouterj · 2013-12-08T12:34:04Z

ping @dantleech

dantleech mentioned this pull request Nov 15, 2013

[WIP] Branch for refactoring to enable actions on content changes. #37

Closed

dbu reviewed Nov 17, 2013
View reviewed changes

wouterj reviewed Nov 17, 2013
View reviewed changes

dantleech and others added 8 commits November 18, 2013 16:52

Example configuration

b2f92bb

Added Config test

3f9ce77

Refactored Configuration builder

0c582f6

Implemented new config

0107391

Removed options new in #37

0ca343e

Added XML support

93cbf9f

Fixed tests

51ff3f6

Added PHP config test

1c444da

wouterj mentioned this pull request Nov 18, 2013

Big config refactoring #36

Closed

wouterj mentioned this pull request Nov 18, 2013

[WIP] Added Config Testing Case symfony-cmf/Testing#27

Closed

Fixed composer and changed internal method name.

4f87d68

dantleech reviewed Nov 18, 2013
View reviewed changes

fixed dependencies

b5913df

dantleech reviewed Nov 18, 2013
View reviewed changes

wouterj added 2 commits November 20, 2013 16:14

Removed rendundant code

df8395a

Made path_units optional (Better Yaml, PHP support)

b5a7272

Fixed test

00c599d

wouterj mentioned this pull request Nov 24, 2013

Updated RoutingAuto configuration symfony-cmf/symfony-cmf-docs#338

Merged

dantleech closed this in f843523 Dec 8, 2013

wouterj deleted the configuration_proposal branch December 8, 2013 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Proposal for new configuration format. #38

[RFC] Proposal for new configuration format. #38

dantleech commented Nov 15, 2013

dbu Nov 17, 2013

dantleech Nov 17, 2013

dbu Nov 17, 2013

dbu commented Nov 17, 2013

wouterj Nov 17, 2013

dantleech Nov 17, 2013

wouterj commented Nov 18, 2013

wouterj commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

wouterj commented Nov 18, 2013

wouterj commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

wouterj commented Nov 18, 2013

dantleech commented Nov 18, 2013

dbu commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

dantleech Nov 18, 2013

wouterj commented Nov 18, 2013

dantleech Nov 18, 2013

wouterj Nov 20, 2013

dantleech commented Nov 23, 2013

wouterj commented Nov 24, 2013

dantleech commented Nov 24, 2013

wouterj commented Nov 24, 2013

wouterj commented Dec 8, 2013

[RFC] Proposal for new configuration format. #38

[RFC] Proposal for new configuration format. #38

Conversation

dantleech commented Nov 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Nov 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Nov 18, 2013

wouterj commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

wouterj commented Nov 18, 2013

wouterj commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

wouterj commented Nov 18, 2013

dantleech commented Nov 18, 2013

dbu commented Nov 18, 2013

dbu commented Nov 18, 2013

dantleech commented Nov 18, 2013

Choose a reason for hiding this comment

wouterj commented Nov 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Nov 23, 2013

wouterj commented Nov 24, 2013

dantleech commented Nov 24, 2013

wouterj commented Nov 24, 2013

wouterj commented Dec 8, 2013