SyliusResourceBundle icon indicating copy to clipboard operation
SyliusResourceBundle copied to clipboard

[WIP] Extract ResourceController actions to separate services

Open Zales0123 opened this issue 4 years ago • 2 comments

Q A
Bug fix? no
New feature? no
BC breaks? no (hopefully)
Deprecations? no
Related tickets
License MIT

This is going to be a big one 🚀

ResourceController is the heart of this bundle and the main class allowing us to do things quickly and easily. Being such a crucial class requires stability and reliability, therefore it's not a surprise it was not siginifantly changed for a long time. Interesting case it is: changes in functionalities can be nicely reflected by changes in specifications, and a lot of them are already 2 or 3 years old ⌛ On the other hand... we can all agree it's not the best written class in Sylius ecosystem 💃 It's huge (over 500 lines, 17 dependencies), as well as it perpetuates bad habits from the past (access to the whole container). Not very solid, if you ask me 🐎 In the meantime, Symfony and PHP applications moved forward, our skills developed, we're already consciously using good patterns, as making controller single-action or using command/query systems for application design. I, personally, have always seen the ResourceController (from the technical point of view), as "the known devil", which may not be written perfect, but we know were it is and how it behaves. Nevertheless, I believe, it's the highest time to move forward.

When thinking about refactoring ResourceController, I came up with some basic goals:

  1. Make ResourceController less scary to look at
  2. Extract as many functionality out it as possible, so it would work as a well-behaved controller, facilitating all the process rather than running
  3. Keep the backward compatibility (probably the biggest challenge)

So, the first step is extracting each of ResourceController's action to separate one-action controllers. As there is a lot of code duplicated between them, I've also started to extract some additional services that would be used in them. To keep the BC, I've introduced a new configuration in the routing generation called controller_type, which should be either single_class or actions. I was hoping to fallback to the actions classes already in the ResourceController, but I guess there is a problem with BC and protected methods - if we fallback by default to the actions' services, overridden protected method would not work in them :| So right now ResourceController stays as it was, until we figure out how to handle this problem nicely.

Let me know what you think about these changes. As you probably see, I'm very determined to make push ResourceController (and therefore the whole bundle) forward, to make it fulfill the highest coding standards. If it's the path we want to go, I believe in this PR we should still:

  • [x] extract some more services to reduce code duplication
  • [ ] document configuration and code changes
  • [ ] deprecate ResourceController and schedule it's removal on v2.0.0 of the bundle

Live long and prosper 🖖

Zales0123 avatar Jan 06 '22 23:01 Zales0123

Thanks for catching this, I'll cut a new alpha release.

ztellman avatar Jan 19 '18 00:01 ztellman