search

jdesrosiers / silex-cors-provider

A silex service provider that adds CORS services to silex
MIT License
78 stars 25 forks source link

Extract "domain" class that would be framework agnostic #24

Closed MacDada closed 8 years ago

MacDada commented 8 years ago

As mentioned here, it would be nice to have a class that would be CORS–oriented, but not framework oriented.

It would prepare response headers, tell if the request is preflight, etc.

It would still be based upon Symfony's Request/Response, but would be Symfony/Silex agnostic (i.e.: configured without knowing of Silex:Application class, would not be a Silex:filter, etc).

MacDada commented 8 years ago

My plan is to make a change that would not be backward compatible:

Is it ok to do that? Considering that ppl installed the lib more than 55k times?

Or I could keep current Cors class name intact, but make Domain\Cors class instead.

jdesrosiers commented 8 years ago

I like the plan. I don't consider the change to be backward incompatible because the Cors class (in its current form) is not something anyone should have been using directly. It is an internal class to the library. Technically, someone could have tried to use it in a way it was not intended to be used, but because everything is private, it would be difficult to use it in any way other than its intended use.

MacDada commented 8 years ago

@jdesrosiers Please see if you like this direction: https://github.com/MacDada/silex-cors-provider/tree/proof_of_concept/src

In the end, I expect current Cors class to be Silex\CorsFilter, containing a few lines. The strategy idea could be used for other aspects, like accepted http methods, etc. – so that new Cors class really just sets headers on Response and nothing else.

Configuration could be done by just composing (strategy) objects. I somehow believe, that it's easier than "magic" configuration keys.

jdesrosiers commented 8 years ago

The strategy pattern doesn't seem like an appropriate fit in this case. One syntax for domain matching should be enough. In fact, there is probably a standard for matching wildcard domains. I envisioned implementing that standard to address the wildcard issue.

I realize that your branch is a work in progress, but I don't like the name "domain". It's too generic. I hate to criticize something without offering an alternative, but I don't have one. We will both have the keep thinking of a better name.

Btw, you can always create a pull request for your work in progress branch. It can be added to or changed as much as we need, but it makes it easier to collaborate. Also, I apologize for not having the time to contribute much more than feedback on this issue. I'll have more time in a couple months.

MacDada commented 8 years ago

One syntax for domain matching should be enough.

If you can implement such mechanism, you can easily register it as another strategy. The point is, the logic can be implemented in any way, but Cors object wouldn't care how. That way, we can test strategies in isolation and add more options, without even touching Cors class.

I imagine someone have a hosts list in the database (and can write a simple strategy class for that).

One could whitelist a domain but blacklist a single subdomain – by composing strategies or writing a new strategy.

It is a lib, it could wait for someone requesting such features, but most of the ppl will just see "it cannot be done" and skip it. It's better if a lib is open to extension in an easy way.

Remember why I am here – I had to copy–paste entire Cors class to change how it works. Luckily OOP design can provide extension points, where implementation gets to be a detail, easily changeable detail.

I realize that your branch is a work in progress, but I don't like the name "domain". It's too generic

Yep, WIP. It was created before you said that changing Cors class is (not) BC.

We will both have the keep thinking of a better name.

I've seen both domain and core names for such stuff. But, it might not be needed to think about such names – if everything goes well, we could just extract that stuff to extra "base" repository:

As you see, I'm thinking quite ahead. It's not that we have such requirements right now – but it would be good to be able to implement future needs without a complete redesign every time something new appears. Especially when it's easy to do (no stable release yet).

MacDada commented 8 years ago

BTW, the "strategies" approach is BC compatible, I mean that it is still possible for the implementation to be configured using silex parameters: https://github.com/MacDada/silex-cors-provider/blob/proof_of_concept/src/CorsServiceProvider.php#L48

So for "simple usages" a default strategy can be chosen, but if anyone needs to change some details, the extension point is to create a custom strategy or compose a strategy using existing implementations.

jdesrosiers commented 8 years ago

it could wait for someone requesting such features

My point was that I don't see this as something that users are going to need to customize. I understand the merits of the strategy pattern quite well, but I also know that it comes at a price in the form of complexity. Simple design is important. I don't want to add the complexity if there isn't real value in doing so.

However, your example of a whitelist stored in a database is a very real example of something that could not be handled well just with just implementing a standard domain matching algorithm. So, I'm warming up to the idea. But, I would like to avoid the verbosity of the full OO strategy pattern if possible.

we could just extract that stuff to extra "base" repository

Yes, this is something I have slowly been making progress toward. This provider was originally extracted from one of my first Silex projects. In it's original form, it was all one class, CorsServiceProvider. If this was a high priority project for me, it would have been cleaned up a long time ago. Instead, I have been slowly refactoring any time a change is needed.

MacDada commented 8 years ago

Simple design is important.

And it can be achieved by making sure that SRP is applied – putting everything in one class is not "simple", but the other way around: "complex" ;)

jdesrosiers commented 8 years ago

I'm not sure what makes you think I was advocating putting everything in one class. I just described to you that I was slowly refactoring away from the original one class solution. SRP was the primary guide I was using to clean up the implementation. I just haven't gotten very far yet.

I realize a lot of the design in this project is not great. Like I said, it hasn't been a high priority for me and it isn't something I show people as an example of my skills. So, don't assume that I don't have knowledge of OO design patterns or SOLID design principles or anything else based on this project.

I also know functional programming. I know that a strategy pattern can be implemented much more simply with functions than with all the boilerplate and verbosity of OO.

I know that putting everything in one class is not simple. I also know that creating 7 classes to describe something that can be expressed with a single well known, standard syntax is not simple either. Simple design is about only adding complexity when there is value in doing so.