UrlShortenerBundle icon indicating copy to clipboard operation
UrlShortenerBundle copied to clipboard

flush in save() method should be limited to just that entity

Open tacman opened this issue 8 years ago • 6 comments

The global flush of the entity manger is unnecessary and problematic -- only the link itself should be persisted to the database. I'll add a PR for this.

tacman avatar Nov 02 '17 10:11 tacman

Hi @tacman ,

Can you explain your use case to have history about that please?

mremi avatar Nov 02 '17 11:11 mremi

We're using the bundle to create a list of links during our testing, and in the process of doing that we're creating entities that we don't want persisted. Alas, some of those entities are tagged in Doctrine as having automatic persistence, which isn't a big deal, since we simply don't flush the entity manager at the end.

So the problem now is that if we use the bundle to create a short URL, it's calling flush() and saving those entities to the database. We want the link flushed, but nothing else.

tacman avatar Nov 02 '17 11:11 tacman

Alternatively, we could pass $flush to the methods that call ->save(), which currently always call it with true. $flush could be passed to shorten() and propogated.

On Thu, Nov 2, 2017 at 7:11 AM, Rémi Marseille [email protected] wrote:

Hi @tacman https://github.com/tacman ,

Can you explain your use case to have history about that please?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mremi/UrlShortenerBundle/issues/12#issuecomment-341389509, or mute the thread https://github.com/notifications/unsubscribe-auth/AAl0QTp7I91nADu_Q_l8dpRRkmjFCowpks5syaNYgaJpZM4QPdk6 .

tacman avatar Nov 02 '17 11:11 tacman

I think it will be more logical to flush only the given entity instead of adding a $flush argument to the Twig helpers.

mremi avatar Nov 02 '17 13:11 mremi

Agreed. The only downside is that flush shouldn't be inside a loop, but that's an edge case. But the save method definitely should not be flushing entities outside the scope of the bundle. Is my PR okay? Alas, I'm not familiar enough with writing tests to do so. Obviously, it's a really tiny change.

tacman avatar Nov 03 '17 11:11 tacman

Let's go to #13 to validate the PR

mremi avatar Nov 03 '17 14:11 mremi