opulencephp/Opulence

Not proper use of prepare statements in the sql repository example on documtentation page

Closed this issue · 4 comments

Hi there,

I have notice some kind of bug or performance issue:

In class Opulence\Orm\Repositories\Repository we can see:

    /**
     * @inheritdoc
     */
    public function add($entity)
    {
        $this->unitOfWork->scheduleForInsertion($entity);
    }

and it seem to be valid.

But I found in your documentation located at https://www.opulencephp.com/docs/1.0/orm-repositories that you propose following code:

    /** @var Post $post */
    public function add($post)
    {
        $statement = $this->writeConnection->prepare(
            'INSERT INTO posts (text, title, author) VALUES (:text, :title, :author)'
        );
        $statement->bindValues([
            'text' => $post->getText(),
            'title' => $post->getTitle(),
            'author' => $post->getAuthor()
        ]);
        $statement->execute();
    }

which seems to be wrong.

Let's imagine that you will need to add 100 posts:

$dataMapper = new PostSqlDataMapper();
// Assume $unitOfWork is already instantiated
$repo = new Repository(Post::class, $dataMapper, $unitOfWork);

for ($i=0; $i < 100; $i++) {
    $postToAdd = new Post($i, 'First Post', 'This is my first post');
    $repo->add($postToAdd);
}

$unitOfWork->commit();

I haven't tried it yet, but from my point of view you will make new prepare statement for each post, instead of make it once and values as many as you want.

I feel this is something that should be considered at the developer's application level not the framework's.

If my app is a tiny stuff, the doc version would be fine. If i am building facebook, i should consider batching db queries.

If you're adding many posts in one go, I think the best approach would be to add another method to your data mapper, eg addMany(array $posts) : void. add() is reserved for single entity additions.

@adelowo It's quite common to have possibility to:

  • create single object
  • import many objects e.g. from CSV

But as @opulencephp wrote: we can use new method addMany(array $posts)

@evolic yeah, i think the addMany method would suffice