core icon indicating copy to clipboard operation
core copied to clipboard

[Eloquent] Add basic data provider / persister

Open alanpoulain opened this issue 4 years ago • 15 comments

Q A
Branch? main
Bug fix? no
New feature? yes
Deprecations? yes/no
Tickets N/A
License MIT
Doc PR TODO

First step of having an Eloquent support in API Platform. No filter and extension (no pagination) for now.

Eloquent support is based on the great WouterJEloquentBundle.

Some inspiration has been taken from EloquentSerializer.

In order to list the properties of the resource (since Eloquent models don't have this information), this PR brings two possibilities:

  • using directly Eloquent models as resources and using a protected $apiProperties class property,
  • using Eloquent models as "data models" and link them to classical resources.

Being "magic" by retrieving the properties in the table, like it's done for instance in larastan, could be done afterwards (but there are some issues, for instance the migrations need to have been executed beforehand). Related library: https://github.com/spatie/laravel-model-info

Only crud.feature and relation.feature are tested for now, the aim is to cover all the Behat tests in the end.

Using Eloquent models as resources with $apiProperties class property

Example:

<?php

declare(strict_types=1);

namespace App\Model;

use ApiPlatform\Core\Annotation\ApiResource;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasOne;

#[ApiResource(
    normalizationContext: ['groups' => ['read']]
)]
class Dummy extends Model
{
    public $timestamps = false;

    protected $apiProperties = [
        'name',
        'description' => ['groups' => 'read'],
        'relatedDummy' => ['relation' => RelatedDummy::class, 'groups' => 'read'],
    ];

    public function relatedDummy(): HasOne
    {
        return $this->hasOne(RelatedDummy::class);
    }
}

Using Eloquent models as "data models" and link them to classical resources

The Eloquent data model should look like this:

<?php

declare(strict_types=1);

namespace App\Model;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasOne;

class Dummy extends Model
{
    public $timestamps = false;

    public static $snakeAttributes = false;

    public function relatedDummy(): HasOne
    {
        return $this->hasOne(RelatedDummy::class);
    }
}

Notice the usage of snakeAttributes in order to avoid issue when normalizing / denormalizing the Eloquent model.

The resource should look like this:

<?php

declare(strict_types=1);

namespace App\Resource;

use ApiPlatform\Core\Annotation\ApiResource;
use App\Model\DummyModel;
use Symfony\Component\Serializer\Annotation\Groups;

#[ApiResource(
    dataModel=DummyModel::class,
    normalizationContext: ['groups' => ['read']],
)]
class Dummy
{
    public int $id;

    public ?string $name = null;

    #[Groups(['read'])]
    public ?string $description = null;

    #[Groups(['read'])]
    public ?RelatedDummy $relatedDummy = null;
}

This data model mapping has been thought to be compatible with all the providers. It should be compatible with Doctrine too (not tested yet).

alanpoulain avatar Apr 08 '21 16:04 alanpoulain

Don't we have an #[ApiProperty] instead so that you could re-use PropertyMetadataFactory?

    properties: [
        'name',
        'description' => ['groups' => 'read'],
        'relatedDummy' => ['relation' => RelatedDummy::class, 'groups' => 'read'],
    ],

I'd suggest something more like:

#[ApiProperty(eloquent=['relation' => RelatedDummy::class])

For names you have the PropertyNameCollection, you could even disable fields with #[ApiProperty(eloquent=false)?

Groups could be Symfony's serializer Group annotation or may there be a conflict in any way?

soyuka avatar Apr 08 '21 17:04 soyuka

@soyuka With Eloquent, most of the time, you don't use a class property or a method for a field, like the Dummy example I added in the description. So I don't think it would be feasible to use ApiProperty or the Groups annotation / attribute.

alanpoulain avatar Apr 08 '21 18:04 alanpoulain

I see and I think we should enforce this. I see the Model classes a reflection of what the API will show. We should start with our API-related interfaces and then use the generated metadata to build the automatic persistence system.Also, this will easy the port between Doctrine and Eloquent or any other system if we need to. For example:

<?php

declare(strict_types=1);

namespace App\Model;

use ApiPlatform\Core\Annotation\ApiResource;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\HasOne;

#[ApiResource(
    normalizationContext: ['groups' => ['read']],
)]
class Dummy extends Model
{
    public $timestamps = false;

    public string $name;

    #[Groups(["read"])]
    public string $description;

    #[Groups(["read"])]
    #[ApiProperty(eloquent=['relation' => RelatedDummy::class])] // btw can't we read this from the below method in a PropertyMetadataFactory bridge with eloquent?
    public $relatedDummy;

    public function relatedDummy(): HasOne
    {
        return $this->hasOne(RelatedDummy::class);
    }
}

soyuka avatar Apr 08 '21 18:04 soyuka

Eloquent models cannot be used like this. Everything needs to go through the magic methods:

  • https://github.com/illuminate/database/blob/fbf9eb746aa8a35054f20658107f6834fcc7be72/Eloquent/Model.php#L1789
  • https://github.com/illuminate/database/blob/fbf9eb746aa8a35054f20658107f6834fcc7be72/Eloquent/Model.php#L1801
  • https://github.com/illuminate/database/blob/fbf9eb746aa8a35054f20658107f6834fcc7be72/Eloquent/Model.php#L1880

alanpoulain avatar Apr 08 '21 18:04 alanpoulain

Hello,

I'm really sorry to write it here.

You've done a lot of work but what's the reason for adding Eloquent in the first place?

Doctrine is much greater and it's stable, doesn't contain sudden breaking changes, and works out of the box with MongoDB.

Eloquent isn't the thing that should be with any PHP framework except Laravel itself. To make Eloquent work you'd be surprised how a lot of things are buggy and it won't be fixed upstream because there is only one person who decides and he never would agree that there is a bug or missing a feature.

Just my two cents.

Thanks!

divine avatar Apr 08 '21 18:04 divine

Hello @divine. The main idea of adding Eloquent support is to use it with Laravel. This PR only brings it for Symfony, but like I said it's only a first step. Anyway, I think it's a good thing to have the possibility to choose your preferred ORM, I think each one has its qualities and defaults.

and works out of the box with MongoDB

Not really :sweat_smile: It was a lot of work to bring compatibility with Doctrine MongoDB ODM. See: https://github.com/api-platform/core/pull/2144. I think Eloquent has also a MongoDB support (not official though): https://github.com/jenssegers/laravel-mongodb

alanpoulain avatar Apr 08 '21 19:04 alanpoulain

Shouldn't we provide a more "Laravely" way to use API Platform with Eloquent?

Laravel massively uses "special" properties to configure how Eloquent objects behaves.

Ex: https://laravel.com/docs/8.x/eloquent-serialization#hiding-attributes-from-json It also uses special methods: https://laravel.com/docs/8.x/eloquent-serialization#appending-values-to-json

Shouldn't we provide an API consistent with this instead?

dunglas avatar Apr 08 '21 19:04 dunglas

Hello @divine. The main idea of adding Eloquent support is to use it with Laravel. This PR only brings it for Symfony, but like I said it's only a first step. Anyway, I think it's a good thing to have the possibility to choose your preferred ORM, I think each one has its qualities and defaults.

Good idea, you're right except for the fact that in a Laravel any PR suddenly might be closed without giving a reason at all, so don't expect something to be fixed with Eloquent if you encounter some bugs with it.

Not really 😅 It was a lot of work to bring compatibility with Doctrine MongoDB ODM. See: #2144.

I meant https://github.com/doctrine/mongodb-odm, of course, you've done a great job at #2144, thank you!

I think Eloquent has also a MongoDB support (not official though): https://github.com/jenssegers/laravel-mongodb

There will be no official integration in the coming years for sure. You've mentioned that library and surprise - I'm taking care of it partially.

It was just a kindly reminder that Eloquent is just a "magic" that just works until you'll realize there are some issues with it. Doctrine is far better in giving an opportunity to do what you want.

Just keep that in a mind, and sorry for writing it here again!

Thanks!

divine avatar Apr 08 '21 19:04 divine

Shouldn't we provide a more "Laravely" way to use API Platform with Eloquent?

Laravel massively uses "special" properties to configure how Eloquent objects.

Ex: https://laravel.com/docs/8.x/eloquent-serialization#hiding-attributes-from-json It also uses special methods: https://laravel.com/docs/8.x/eloquent-serialization#appending-values-to-json

Shouldn't we provide an API consistent with this instead?

Yes, I thought that too. Maybe a $properties property and/or a properties method in the model. I don't have a strong opinion about it. This is a choice between Laravel-like and API Platform-like.

Another option would be to have both ways to list the properties.

alanpoulain avatar Apr 08 '21 20:04 alanpoulain

It was just a kindly reminder that Eloquent is just a "magic" that just works until you'll realize there are some issues with it. Doctrine is far better in giving an opportunity to do what you want.

If you are right (and since you take care of the MongoDB support in Eloquent you certainly are!), we will probably not have 100% of the features for Eloquent like Doctrine, and we will probably have some difficulties for maintaining it in the long term. But I personally think that supporting Laravel in API Platform would be a great improvement and that's why I've started to work on it :slightly_smiling_face:

Since you seem to know well Eloquent, I will probably ping you for the future PRs if it's OK with you?

alanpoulain avatar Apr 08 '21 20:04 alanpoulain

What about something like this (code not tested, based on an API proposal by @soyuka)?

namespace App\Api;

use App\Models\Book;
use ApiPlatform\Core\Annotation\ApiResource;

#[ApiResource(eloquentModel=Book::class)]
class Book
{
    public string $id;
    public string $title;
}

The corresponding data provider could be implemented this way:

<?php

namespace ApiPlatform\Core\Bridge\Eloquent;

use ApiPlatform\Core\DataProvider\ItemDataProviderInterface;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;

class EloquentDataProvider implements ItemDataProviderInterface
{
    public function __construct(
        private ResourceMetadataFactoryInterface $resourceMetadata,
        private PropertyNameCollectionFactoryInterface $propertyNameCollection,
        private PropertyAccessorInterface $propertyAccessor
    ) {}

    public function getItem(string $resourceClass, $id, string $operationName = null, array $context = [])
    {
        $eloquentClass = $this->resourceMetadata->create($resourceClass)->getAttributes()['eloquentModel'];
        $eloquentObject = $eloquentClass::find($id);

        $apiResource = new $resourceClass;
        foreach ($this->propertyNameCollection->create($resourceClass) as $prop) {
            $this->propertyAccessor->setValue($apiResource, $prop, $eloquentObject->{$prop});
        }

        return $apiResource;
    }
}

dunglas avatar Apr 08 '21 21:04 dunglas

But I personally think that supporting Laravel in API Platform would be a great improvement and that's why I've started to work on it 🙂

It's actually a good idea! Laravel has its own ecosystem and a lot of people use it.

Since you seem to know well Eloquent, I will probably ping you for the future PRs if it's OK with you?

@alanpoulain sure, would glad to help but I can't say that I'm better than you at Eloquent - it sits back in its own world 😃 .

Thanks!

divine avatar Apr 10 '21 07:04 divine

Just my 2 cents: Back in 5.x days of Laravel, supporting Eloquent outside the Laravel framework was indeed very difficult and required quite a lot of hacking. Since the introduction of semver in Laravel 6.0, I've found adding support for newer versions to be much much easier. I even changed the version constraints from a specific minor to a specific major in my bundle.

Upgrading from major to major has been smooth sailing as well:

  • 6->7: https://github.com/wouterj/WouterJEloquentBundle/commit/10ea743cfc30fa0fa4cfe3a7b0670d97eed79204 - note that most of them are because I'm using internal maker stubs from Laravel;
  • 7->8: https://github.com/wouterj/WouterJEloquentBundle/commit/8a10ab9165b8fe9fad212e9612b4bc5c766b67e6 - only 1 internal stub related change

In other words: I'm quite positive about maintaining support for the newer Eloquent versions once the implementation is build (and respect to @alanpoulain for going through the initial work on that!).

wouterj avatar May 11 '21 22:05 wouterj

Hello @wouterj. Thanks for your input. It's great to know that the Laravel ecosystem is becoming more professional since the introduction of semver and that the bundle will follow the future Eloquent versions as well!

alanpoulain avatar May 17 '21 15:05 alanpoulain

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Nov 14 '22 01:11 stale[bot]