Skip to content

Conversation

ElectricMaxxx
Copy link
Member

@ElectricMaxxx ElectricMaxxx commented May 2, 2016

Try to let the enhancers work in a HTTP method aware manner as discussed in #171

@ElectricMaxxx
Copy link
Member Author

Failing test looks random, but happened the second time tonight.

@ElectricMaxxx
Copy link
Member Author

ElectricMaxxx commented May 3, 2016

Maybe i should create a separate enhancer that works before FieldByClass to normalize the defaults value that FieldByClass can work the way it used to work before. Means to create `"My/Class" => "Service:method"

But the enhancers have no prio ...

@dbu
Copy link
Member

dbu commented May 4, 2016 via email

@ElectricMaxxx
Copy link
Member Author

ElectricMaxxx commented May 4, 2016

keeping the FieldByClass enhancer clean is my idea (last comment) too. But as usual: you have got great ideas, but i don't understand them on the first time. :-)

I try to refrase it:
You mean that conditional enhancer should wrap all others. No HTTP method would mean that the others can to their work in the chain as they used to do. With an HTTP method (i.e. POST) we need to transform the configuration like that a version the former enhancers still understand like

...
controllers_by_class:
     My\Class: "service:method"

(i would pass it as an configuratio, but the enhancer understands an array of class => service:action)

Doing that transformation would keep the other enhancers as they are. Is it that what you mean?

@lsmith77 lsmith77 added review and removed wip/poc labels May 4, 2016
@wouterj
Copy link
Member

wouterj commented May 8, 2016

Failing test looks random, but happened the second time tonight.

See #173

@ElectricMaxxx
Copy link
Member Author

ElectricMaxxx commented May 8, 2016

@dbu slowly i get your idea, but wouldn't it change the current call behavior and the extension class at all? Currently we are passing the configurations directly into parameters and pass them to the enhancers while their service configuration. What you plan is a thing between the ExtensionClass and the DynamicRouter, which should prepare the configuration values. And that conditional enhancer can be a kind of a chain enhancer, which is called by the dynamic router, which doesn't recognize the change. But don't you think this should be done in a separate Issue/PR?

@dbu
Copy link
Member

dbu commented May 9, 2016 via email

@ElectricMaxxx
Copy link
Member Author

ElectricMaxxx commented May 9, 2016

@dbu but some the enhancers expect some parameters we have to be aware of. Even a conditional Enhancer need those. A conditional enhancers need the Information like POST goes on service:postAction to feed the FieldByClass enhancer, that does its job as it used to do. I am completely with you by keeping the enhancers clean as they are. But i would not do some kind of method => enhancer mapping in the code. I think we should pass the name while its Service Definition and ask later isYourName('controller_by_class') while looping through them all. A so called ConditionalEnhancer would do some Kind of normalizing, when the parameters are Method aware. Doing so the enhancers stay, their service derinitions are reduced and we get the conditional behavior.

@ElectricMaxxx
Copy link
Member Author

ElectricMaxxx commented May 9, 2016

@dbu i hopefully did what you whanted. I moved the enhancer sorting and loopging through the enhancers from the DynamicRouter to a ConditionalEnhancer. For regular enhancers it works as before. For the enhancers that need parameters i created an interface which forces them to do a name comparison (am i responsible for a set of parameters) and to implement a setter for the mapping/values/parameters passed to the the enhancer. Those parametes do not need to be passed throug the service defintion anymore. The ConditionalEnhancer takes them all and pass them to the enhancers by asking them "is that yours" (isName()). Enhancers that should have got mappings, and do not get them, won't be used.

That comes with several advantages:

  • no need to do the tagging of parameter depending enhancers in the extension bases of the configuration
  • no need to pass the separated configuration parameters through the service, just pass all under dynamic_router into the ConditionalEnhancer
  • chance to "normalize" the parameters based on a HTTP method in a generic way (all configuration can have a methods mapping now, no matter if that makes sense)
  • the enhancers stay stable in their core behavior.
  • DynamicRouter depends on an abstraction now, doesn't do it on its own
  • generic solution which opens doors for other enhancers, whithout changing code
  • configuration in routing bundle can be generic too, key (controller_by_class) needs to match a name of a registered enhancer (cmf_routing.enhancer.controllers_by_class) only

I hopefully get into the direction you whanted it, at least i hope this can be a starting point for a refactoring without reverting to much stuff.

PS: Some stuff (docblock, tests) is still missing.

@dbu dbu mentioned this pull request May 10, 2016
@dbu
Copy link
Member

dbu commented May 10, 2016

i tried to show what i imagined in #174

@ElectricMaxxx ElectricMaxxx force-pushed the rest_routes_on_controller_by_class branch from 8c026c4 to 56fed00 Compare May 10, 2016 21:08
@ElectricMaxxx ElectricMaxxx changed the title let enhancer work with http method aware defaults [POC] let enhancer work with http method aware defaults May 10, 2016
@ElectricMaxxx
Copy link
Member Author

solved in #174

@lsmith77 lsmith77 removed the review label May 12, 2016
@dbu dbu deleted the rest_routes_on_controller_by_class branch November 30, 2016 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants