Used Doctrine OM for batch removal

[bobvandevijver]

When batch deleting entities, a direct query on the database is used. In Doctrine environments which do not allow the DELETE command on the database, it is not possible to use the batch delete command, while the single delete works fine.

This is due to the fact that soft-deletables register on the onFlush event. There might even be more listeners that listen on object deletion, which are not executed with the current code. This PR should solve this for Doctrine.

I have no clue how this might work in DoctrineODM or Propel, so I've only adjusted the Doctrine code.

[sescandell]

I understand your point, but I don't agree with this change. In many other situations I can't accept making several requests (without taking into account the additional memory and timeuse to get entities from DB) just to handle softdeletable objects.

In my opinion, if you have specific needs that requires to not use DELETE SQL query, you should:

• Override the method in your application if this suits to a given entity
• Or override the template in your app to use a workaround as you propose in this PR

Maybe something we can do to satisfy everyone: make an option to the DeleteAction to precise: use batch or unit call. What do you think?

[bobvandevijver]

I prefer to use the entity manager always when it is about lifecycle related operations: the only thing I use DQL/the querybuilder for is to select data, as when they are converted into objects the load lifecycle is still called.

However, I also see your point. Lets make it configurable on a global level, which can be overwritten in the specific generator configuration. I will update this PR with the functionality.

[bobvandevijver]

@sescandell How about this?

[sescandell]

That works.

But I was initially more thinking about an option in generators than in global configuration.

Which one do you think would be the best?

[sescandell]

Another question: what about Propel? I imagine, there is something similar to this issue, is there any way to do the same thing for it?

[bobvandevijver]

I doubted between the global configuration and the per generator config, but I choose for the global configuration as it is more logical to enable this in one go for every case. If you want to use the Object Manager for your removal (for instance for the soft-deletable) you want to enable it in one go for everything, instead of enabling it in every separate generator config file, with the risk of missing some of forgetting it when adding new generators.

For the other managers: I have no clue on how those work. Therefor I invite others to make a suggestion, then I can incorporate it :)

[sescandell]

I doubted between the global configuration and the per generator config, but I choose for the global configuration as it is more logical to enable this in one go for every case. If you want to use the Object Manager for your removal (for instance for the soft-deletable) you want to enable it in one go for everything, instead of enabling it in every separate generator config file, with the risk of missing some of forgetting it when adding new generators.

There might be situation where you only want to activate softdelete on "main classes"... but let it like that. It should be okay :+1:

For the other managers: I have no clue on how those work. Therefor I invite others to make a suggestion, then I can incorporate it :)

You can look how we handle that here: https://github.com/symfony2admingenerator/GeneratorBundle/blob/master/Resources/templates/Propel/ActionsBuilderAction.php.twig#L29

[sescandell]

Need a rebase

[ioleo]

For Propel you simply add one of the following methods to object class:

• preInsert() code executed before insertion of a new object
• postInsert() code executed after insertion of a new object
• preUpdate() code executed before update of an existing object
• postUpdate() code executed after update of an existing object
• preSave() code executed before saving an object (new or existing)
• postSave() code executed after saving an object (new or existing)
• preDelete() code executed before deleting an object
• postDelete() code executed after deleting an object

More info in the sf 2.0 docs - this information was removed in v2.3.

A direct SQL query will (as in Doctrine) bypass any lifecycle callbacks. I will update this PR and contribute the Propel implementation today.

[bobvandevijver]

@loostro :+1:

[ioleo]

@bobvandevijver can you rebase and resolve conflicts before I start my part?

[bobvandevijver]

@loostro Done!

[ioleo]

@bobvandevijver done, the propel implementation now iterates over objects and calls delete... however the "batched" solution in propel also calls delete (only, on a collection). I think they do the same (and the code is reduntant), but I'm not sure current "batch" implementation will execute only one query.

Probably we should create and execute a single SQL "DELETE" statement instead.

[bobvandevijver]

@loostro Well, I can not test it, as I do not use propel :smile: Can't you test it on some data and check what database calls are made?

[ioleo]

Squashed into single commit. Propel implementation needs testing and probably changeing the else block.

[sescandell]

:+1:

One suggestion: the generated file could be improved by taking into account (on generation) the batch_remove parameter. So we can immediatlly have:

    protected function executeBatchDelete(array $selected)
    {
            $em = $this->getDoctrine()->getManagerForClass('{{ model }}');
            $objects = $em->getRepository('{{ model }}')
                ->findBy(array('{{ builder.getFieldGuesser().getModelPrimaryKeyName(model) }}' => $selected));
            foreach ($objects as $object) {
                $em->remove($object);
            }
            $em->flush();
            $em->clear();
    }

or

    protected function executeBatchDelete(array $selected)
    {
            $this->getDoctrine()->getManagerForClass('{{ model }}')
                ->createQuery('DELETE {{ model }} m WHERE m.{{ builder.getFieldGuesser().getModelPrimaryKeyName(model) }} IN (:selected)')
                ->setParameter('selected', $selected)
                ->getResult();
    }

(No test and simpler generated code).

Just a suggestion :)

[bobvandevijver]

@sescandell I agree, I've added it. So, is it ready for merge, or do we need a propel tester?

It's not BC-breaking, so I see no harm in merging.

[ioleo]

:+1: I think it's OK to merge.

Rebased to current master.

[sescandell]

:+1: Good for me !

[bobvandevijver]

Merged, thanks all :smiley: