Don't use private properties on the Delivery Client about contentful.php HOT 15 CLOSED

Added a pull request to demonstrate this.

#8

from contentful.php.

Hi Aron,

the extension story for the SDK is admittedly somewhat unrefined. Long term the plan is to allow you to override the ResourceBuilder, whether that's through a factory method or dependency injection is not quite clear. The reason why you can't do that today is that the interface of the ResourceBuilder is far from stable.

The other part of this is that we want to allow generating custom classes to use with the ResourceBuilder see the ClassGenerator. That's not yet enabled since the generated classes are still buggy and I haven't gotten around to fixing them. Customising them would also rely on a bit more stable interface for ResourceBuilder.

I'd be curios why you're trying to override ResourceArray. Maybe you could share a bit more about what you're trying to achieve?

Cheers,
Rouven

from contentful.php.

Hi Rouven,

Thanks for the speedy response. I'll clarify my use-case a bit. I'm using Contentful as the backend for a Laravel-driven website and want to model my data using simple models. I made a ContentfulModel that talks to a custom query builder. Reason for doing this is that I can now define and retrieve my content like this:

class BlogItem extends ContentfulModel
{
    protected $source = 'blogItem';
}

Which can then be used to retrieve content using BlogItem::all() or queries like BlogItem::where('fields.slug', 'foo')->get(). This make my code a lot more expressive.

The problem, however, is that the get() method eventually proxies to Contentful\Delivery\Client::get which will then return a ResourceArray. In my ideal situation, it would return a BlogItem model.

There are several reasons for wanting this: validation for example, but also simple overrides. I'm a big fan of Carbon for date manipulation, but the DynamicEntry type returns DateTimeImmutable objects for example. In order to localize dates, I now have to retrieve the date and pass it to strftime.

Regarding the ResourceArray type: I haven't missed any features so far, but for the sake of consistency, overriding it would really benefit my application because Laravel has it's own Collection class.

from contentful.php.

Hi Aron,

I had to think about this a bit. As I see it you basically want to implement an Eloquent style interface for Contentful. Correct? I admittedly based the design of the SDK more on Doctrine which might be a bit jarring for someone more comfortable with Eloquent.

I don't think we'll offer the kind of extensibility necessary to achieve that. The simple reason is that the SDK is trying to be forward compatible with adding stricter type validation once we go PHP7 only. I'll have to play around a bit with the Eloquent base classes to see if they could satisfy an interface extracted from ResourceArray. In any case I'd open to adding some convenience methods to ResourceArray.

As for Carbon, the problem is that it's instances are mutable. This is completely incompatible with the way the SDKs cache works right now (it caches instances). This is going to change in the near future but even then I'd like to guarantee that the value objects contained in an Entry are immutable. (See also briannesbitt/Carbon#406

All of this is of course not a solution to your problem. One thing that could work is a "raw mode" that takes care of all the querying and authentication but does not build objects from the result. Would that be helpful or is that too much effort? The object hydration part is admittedly the biggest part of the SDK.

Cheers,
Rouven

from contentful.php.

Hi Rouven,

I think a good (and pretty easily implemented) starting point would be to convert any private methods to protected methods. This would at least allow developers to extend the library to their own wishes. From that, good stuff will probably/hopefully emerge, enabling you to make choices about the direction this API should move to.

Especially the instanceCache property is quite an important one. Without being able to override it, things get sluggish really fast, especially when retrieving related models.

from contentful.php.

I agree with the above, almost a year on. Having just gotten into this SDK, there are a lot of things I would like to extend, where I'm stuck extending the entire object. One example is I would like to be able to wrap my results in a collection, instead of your custom array objects.

from contentful.php.

I'm closing this issue because the solution won't simply be to mark properties as protected rather than private. During early 2018 I will be working to change some things in the SDK, and this will be one of them 🙂

from contentful.php.

@dborsatto sorry to bring this up again, but after a several year hiatus, I am currently integrating Contentful in another project and am still stumbling upon this exact same issue.

You mentioned that improving extensibility was on the roadmap for 2018, but as far as I can see, it is still impossible to extend the ResourceBuilder. Am I overlooking something?

from contentful.php.

Hey @ArondeParon
I haven't worked for Contentful for more than 2 years, so I can't be of help. I'm pinging @pgrigoruta because it looks like he's the one committing on this repository now.

from contentful.php.

@dborsatto oops, I did not notice that! Thanks for getting back to me so far.

Goal

Being able to extend ResourceBuilder so that I can create simply value objects for my content.

Blockers

• The delivery client currently has a public getResourceBuilder() method that sits unused. By default, it returns the private $builder property.
• All the internal code does not use getResourceBuilder(), but the $builder property directly, which makes extending only possible when extending everything.

Proposed solution

Stop using $this->builder directly, and use getResourceBuilder().

from contentful.php.

Hi @ArondeParon ,

@pgrigoruta actually stopped working for Contentful and I'm in the process of taking over the maintenance of the SDK. I don't have the overview yet to have a good opinion on this, but your proposed solution seems good on the first look. I'll reopen this issue for tracking :)

from contentful.php.

@Sebb767 thanks for the quick action! I have created #298 that implements this ability in a backwards compatible way. Would love to hear your feedback!

from contentful.php.

@ArondeParon Thanks for the PR! :)

It looks good for me. I'll check back with the other maintainers and get back to you in the course of this week (sorry if it takes a while, I'm still in the process of taking over!).

from contentful.php.

Hi @ArondeParon , did the discussion in #298 solve your issue or do you still need help on that one? :)

from contentful.php.

Hi @Sebb767, it did not quite solve my issue, but it looks like there is no need for my PR at this time, since custom mappers can be added to the builder.

I do not use this solution myself, but I can imagine that it does the trick for others 😊

from contentful.php.