search

prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language
https://prismic.io
Other
109 stars 83 forks source link

Pass linkResolver to htmlSerializer #108

Closed tremby closed 7 years ago

tremby commented 9 years ago

This makes for more flexibility and reusability of htmlSerializers

See #106, part of which was this change.

erwan commented 9 years ago

Hi Bart,

Thank you for your PR - however when doing this kind of API change, we first need to decide to do it for all kits.

So it's unlikely we merge this one, but you can still write an htmlSerializer that takes a linkResolver in parameter by using a higher order function (e.g. a function that takes a linkResolver in parameter, and returns an htmlSerializer with only the $element, $content parameters).

tremby commented 9 years ago

I'm surprised. I don't see why you'd allow one to be passed but not the other.

Doing things the way you suggest seems extremely awkward in comparison to adding an extra optional parameter. On 20 May 2015 00:49, "Erwan Loisant" notifications@github.com wrote:

Hi Bart,

Thank you for your PR - however when doing this kind of API change, we first need to decide to do it for all kits.

So it's unlikely we merge this one, but you can still write an htmlSerializer that takes a linkResolver in parameter by using a higher order function (e.g. a function that takes a linkResolver in parameter, and returns an htmlSerializer with only the $element, $content parameters).

— Reply to this email directly or view it on GitHub https://github.com/prismicio/php-kit/pull/108#issuecomment-103799517.

erwan commented 9 years ago

The reason is that most of the time you have a single linkResolver in your project. So you can use the linkResolver from the context in your htmlSerializer, and that keeps the signature simpler.

tremby commented 9 years ago

But it complicates the htmlSerializer. Now instead of being reusable in multiple situations and across projects it needs to directly reference or instantiate a linkResolver, which is likely to be different between projects.

If you are thinking about it that way, i.e. a single linkResolver per project, why allow the linkResolver to be passed at all? Why not have a linkResolver instance passed to the kit as part of the configuration procedure? That would make the signatures of a great number of methods simpler, and at the same time would simplify a lot of my code. I think this would be a change for the better, but obviously would be a breaking change. Anyway, as it stands, I need to have these instances globally available all the time.

I actually try to get around that. I have a reusable wrapper around most of the kit which handles automatically passing in the project's linkResolver in as many cases as I can manage. The only case left, which I have to have this reference lying around for, is each time I use an htmlSerializer.

Because as you said, it's the linkResolver which stays the same. And for me the htmlSerializer changes from call to call. I have a lot of htmlSerializer instances which are reused here and there and they're simpler and more portable if I don't have to reference the linkResolver in each one.

Having the kit pass the resolver through (the developer has just passed it to the asHtml call; of course he wants the same one available in the custom serializer as will be used by the built-in code!) adds consistency, and for me makes the API more useful and intuitive.

As far as I can tell, it's a very small change with a lot of benefit, and I don't see how it could break anything. On 20 May 2015 01:10, "Erwan Loisant" notifications@github.com wrote:

The reason is that most of the time you have a single linkResolver in your project. So you can use the linkResolver from the context in your htmlSerializer, and that keeps the signature simpler.

— Reply to this email directly or view it on GitHub https://github.com/prismicio/php-kit/pull/108#issuecomment-103803796.

tremby commented 7 years ago

Any reason? I think I made a lot of valid points here.