search

craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.25k stars 631 forks source link

need to avoid 3.1 recoverable elements for plugin cases #3392

Closed narration-sd closed 5 years ago

narration-sd commented 5 years ago

Description

Hmm. For what-you-know, I need to create elements on the fly and soon destroy them. With 3.1, you have the new trashable/recoverable/garbage collected elements.

I've dug down as far as to seem to see no interface for setting an element and all its possible children to be trashed, or to extend deleteElementById() to assure the same.

I'd prefer deleteElementById() to have a defaulted false argument to do this, as it's clean and also won't involve prior resaves of elements and children created by duplicate(), as one example I use.

Thanks...

Steps to reproduce

  1. create element
  2. delete it within same session
  3. for proximate forever, until gc, it will still be there. My database is filling up....!

Additional info

narration-sd commented 5 years ago

p.s. I didn't add any of the soft delete aspects for these elements, because I don't want that. But the code in Element::deleteElementByID still seems to check and silently fail to delete the master record or its children.

Another way to look at the problem...that in fact soft delete/trash-gc isn't actually optional??

brandonkelly commented 5 years ago

I don’t think we’ll add the ability to immediately hard-delete elements from the service, but your code can just do this after calling Elements::deleteElement():

Craft::$app->db->createCommand()
    ->delete('{{%elements}}', ['id' => $theEntryId])
    ->execute();
narration-sd commented 5 years ago

Ok, and will this assure child elements get deleted also?

I'm particularly concerned with plugins like Neo...some trouble with that.

brandonkelly commented 5 years ago

It would only partially delete Neo blocks, assuming its table has a cascade-delete foreign key to the elements table. Some Neo block data would hang around until it got garbage-collected after 30 days, but it won’t be visible or show up in queries or anything, so that shouldn’t matter.

narration-sd commented 5 years ago

That's what I thought. I will guess customers can deal with/won't notice that, since they not too eventually go away. It may be better than current or at least recent state per Neo.

narration-sd commented 5 years ago

Hmm. Brandon, could I ask you to think on this, once more in the light of day?

I gave this more of my night, and implemented, cleaning some inevitables. It works for the primary record, but leaves a slew of things around -- Craft Matrix as well as current state Neo.

I think this is too much to bear for persons who will implement Live Vue while building up a site , maybe needing to look the db to see what theirs or other plugins are doing.

I don't really want to write an element walker, when a simple flag passed through Element->deleteById() would do the job...

Make sense? Pic of the present carnage enclosed. Top group of six shows the last time before fix in, /after that, groups of 5 for each Live Preview update. And there will be all the content records....

This seems really far too much to have, much less wait on for a month, don't you think? And if you met such yourself?

element-delete-leftovers

narration-sd commented 5 years ago

p.s. thanks. And I have to use the Preview here more often, for cleaner emails...habits die hard...

narration-sd commented 5 years ago

Or, just as I truly shut down and look away, thought comes it could be an Event from inside the delete/s. Would be as happy.

The point is to defeat the new hold-til-gc of 3.1, when not apropos, let us cleanly and fully as can delete records. I think Craft was actually handling that for Neo even, anyway, quite possibly from code I saw....

narration-sd commented 5 years ago

p.p.s. that's the elements table, and again, 5 records plus all their children left over, for each screen update/typed letter or few when in Live Update...

brandonkelly commented 5 years ago

It works for the primary record, but leaves a slew of things around

Right, as I said, some Neo block data will hang around until it gets garbage-collected after 30 days (and yes that also applies to Matrix blocks). That’s because the foreign key lives in whatever table is used to define Neo/Matrix specific properties (in Matrix’s case that’s matrixblocks), so a cascade-delete on that isn’t going to bubble up to the elements table for those block elements, or any other rows that have FK’s pointed at the other elements table rows.

But I really don’t think people are going to mind extra database data lying around. MySQL and PostgreSQL won’t sweat the extra data, and it won’t ever actually get queried by element queries. I think it would be a bigger issue if that data wasn’t going to get hard-deleted ever, but knowing that it will eventually be hard-deleted makes it tolerable.

brandonkelly commented 5 years ago

If anything I think people might be concerned with why all these extra elements are getting created in the first place, which will become more obvious if some of the data is left in-tact for a while. Hard-deletion isn’t a real solution to that though; it’s just sweeping the problem under the rug. It would be much better if the plugin didn’t create so many elements to begin with.

narration-sd commented 5 years ago

Well, I don't create 'so many elements'. Craft does, when I create one dupllicate of one master element, specifically an Entry.

I need to create that one temporary Entry, then delete it, so that Craft's cousins, element-api and CraftQL, have something to chew on.

I woke up with the insight, background processing while certainly not dreaming about it, don't do such, that maybe I could and thus should avoid this and a number of complex iron-clad-on-faults interactions by extending the transaction I guarantee duplication steps with, to run over the entire Craft transaction.

If it were to work, it would hide this gc-needing addition without affecting its attempts, because the created records would simply disappear, never having been seen. I think.

I don't know whether Craft dbs (or any call needs) handle nested transactions, which could stop this. Any thoughts on that? One grace is that there shouldn't be many, because my Craft operations are read only. Which means that precluding CraftQL from upserts etc. by this shouldn't be a problem.

Brandon, you've seen the base tech for my plugin run 9 months ago. I've had to refactor and surround with demanding finess all that it invokes, for at least five of those months. Four went to decreasing circles on an api-decoding/writing scheme that turned out to be a bad idea, much replaced by the ease people will have with CraftQL, while both are supported native as they are.

The elaborate JS/Vue library, the many Craft-internal-modifying Events, Behaviors, and lower-case behaviors, are what has taken the rest of the time. This is like the silent months/years Craft went through developing your own multiple internal multifacet solutions, because I've had to get inside very many of those that you fellows worked out.

The goal is to make something that fundamentally works very well, also both non-fragile, and non-intimidating. I ask those questions consistently once a week, after another round with working app models. The fragile part gets tested as you mutate Craft, not to say the moving target of ES6-able build environments and Vue itself.

I look to you for some ease, and you've often provided great ideas, if understandably resistant on actual Craft changes. The middle ground we've hit has come with your growing Event/Behavior flexibility. For other participants, I've had to break down similarly complex architectures and write fully functional PRs to get a solution they better like engaged.

I've tried that move once or twice here, taxing as it can be -- I am only one person. I appreciate that you like to stay with your own creativity, and appreciate the many motivations.

In this case, very near where I can actually put this thing out for a soft beta with experts to see how it stands up vs. potential support, I don't yet see why you wouldn't think to make a new activity of Craft optional. That's all I'm asking for.

I'll spend some looking into possibly nested transactions, and the consequences of refactoring yet another fundamental mechanism of my own. Maybe you could think on this small mod to 3.1?

Thanks, Clive

narration-sd commented 5 years ago

p.s. as I always have them. Of course there can be arbitrary many Entry child elements created -- because Craft is that way(!), and people exploit it.

I keep reading so, in all the 'other' things persons come to Support about...and you know.

narration-sd commented 5 years ago

p.s. as I always have them. Of course there can be arbitrary many Entry child elements created -- because Craft is that way(!), and people exploit it.

And, Neo is and has been its own case, thus especially good to be able to just discard what it's done on all counts, no?

narration-sd commented 5 years ago

And, a look at nesting transactions seems to throw this out the window.

You can write them, but the surrounding one will be auto-committed when you start another.

Thus Craft-internals such as the constant queued-task setups due to db operations will kill my surrounding transaction, leaving the gc-nee tding extras present for Craft 3.1-plus.

I didn't reply about the effects of this burden that can grow like topsy, but I think if you apply the evident minds of Tim, Carl, etc., to this, it seems rather likely they'd not feel right about using such a plugin.

And as in Ben's recent issue, what will an end-customer think, when there's some other problem frightening them, and/or they note the extent of db backups, and in either case realize something unexpected and of size is afoot...

narration-sd commented 5 years ago

p.s. apologies for the title - humor of a late post-fixes moment, based on your internal naming, no disrespect intended, and I've changed it

narration-sd commented 5 years ago

Brandon, I don't like to keep filling up your email with these, but thinking is on hard run status, as this keeps feeling critically important to my project.

There's no use releasing if it's going to look bad in a month or two, nor wasting people's time in proving out achieved usability of all the infrastructure built for its offer, if project is going to need to be walked away from....

Here's a possible idea for reducing your effort, if it indeed would run efficiently enough.

And, followed by what I think is a much nicer suggestion, but this first so you know I'm thinking. Or you could just skip to the bottom...

Undesired iif intriguing way:

How efficient/heavy? Do we want to find out? I don't think so -- it's just creating more awkwardness.

Hence this much lighter weight question/solution:

Wouldn't just reintroducing the original full-delete code selected on an extra argument to both routines such as $doHardDelete be a nice way, and not hard? And no efficiency issue at all, as I long ago proved that code no problem.

I guess that would be my actual PR, if you want me to write one to short the effort on this. I always know you are likely to come up with something more elegant, but no need for elegance here?

Trying to get the mind off the tendency for afterburner, at least, so posting.

narration-sd commented 5 years ago

...added in edit: I always know you are likely to come up with something more elegant, but no need for elegance here?

p.s. via shower: there's an even nicer way for my proposal -- work the switch to current fully-deleting code only in deleteElement, and get the flag from the Event remembered is there.

I'd activate the flag only when needed, of course.

This way, better aligned with your preferences as I may understand them, no flag arguments on functions, less interference, simpler.

narration-sd commented 5 years ago

Hi Brandon -- listen, I've just built a fix for this, and will PR it in a moment given no repo issues.

After thinking about it, took less than an hour to write and test.

I hope this justifies the request, and that you'll accept it.

Just keeping the overload of records from disturbing any software/installation troubleshooting should I believe make this extremely worthwhile -- to developers, and to end users of Craft. Best basis I can consider...

Thanks, and for patience, Clive

narration-sd commented 5 years ago

as a side note, but a useful one, this fix looks entirely successful at getting out all the Elements creations of Neo fields. There's no difference in the elements table after multiple Live Preview refreshes which invoke Live Vue's necessary temporary hidden Entry duplications.

It also quite simplifies what had begun to be added to the plugin's hidden record handling, so there's that also for potential future needs-to-use.

And yes, I should have named the branch '3.1 recover Element full delete', but you can do that if it makes a smile at the merge ;)

narration-sd commented 5 years ago

Gratis indeed for taking this up, Brandon.

I'll have your version of the accepted pull in place, up in the morning....

narration-sd commented 5 years ago

and thought I could save you closing this issue, but guess not ;)

narration-sd commented 5 years ago

and...up and tested, works fine in 3.1.x-dev.

Once more, thanks, Brandon.

Mystery of the moment is why I couldn't composer update to craftcms/cms's direct 3.1 branch instead of the 3.1.x-dev which I think always comes from packagist, even though I put in a vcs repository item in for it and tried dev- incantations etc., but we'll let that go I guess ;)

At least I understand why I couldn't close this at o-dark-30 -- of course it was already closed...

brandonkelly commented 5 years ago

Ended up needing this for the new element draft implementation in 3.2, so saveElements() has a $hardDelete method now.