craftcms / cms

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

Feature request: Improve behaviour for automatic resaving of entries when saving an entry type #3482

Closed mmikkel closed 5 years ago

mmikkel commented 5 years ago

Description

In Craft 3, saving an Entry Type's settings (e.g. in order to add a new field to its field layout) triggers a resave of all entries belonging to that section/entry type.

Sometimes a resave is necessary – e.g. if you change the URL format – but if you've a lot of entries, the re-save can be a problem, due to the time and resources it takes to actually resave all of the entries. Also, a lot of the time (more often than not, from my perspective) a resave isn't even needed – such as when you simply re-arrange the fields in the field layout.

It'd be great if

  1. There was a config setting to disable the automatic re-save (could/should be true by default, i.e. keeping the current behaviour intact). Developers would then be able to implement their own strategies for dealing with changes to an entry type, whenever a re-save is actually needed.

  2. If automatic re-save is enabled (or the config setting isn't implemented), Craft could be smarter about when to actually do it (i.e. changes to the URL format should trigger a re-save; simply re-arranging the fields in the field layout should not).

  3. A native feature for manually triggering bulk resaving of entries belonging to a particular section or entry type would be awesome.

Additional info

brandonkelly commented 5 years ago

As you pointed out, there's times where it's definitely necessary, so I don't think giving people a way to completely disable it is a good idea. But your second suggestion is good.

mmikkel commented 5 years ago

@brandonkelly I don't disagree, although it could be argued that such a config setting would simply be one of those things that a developer shouldn't mess with unless they know what they're doing (much like setting the runQueueAutomatically setting to false).

If automatic re-save was disabled, it'd be the developer's responsibility to both a) determine if a re-save is necessary and b) actually re-save the entries, somehow.

theskyfloor commented 5 years ago

Number 3 would be fantastic... I use preparse a lot and I rely heavily on re-saving of elements to update the parsed data.

ryanpcmcquen commented 5 years ago

Another vote for the ability to re-save all entries. We have an internal plugin that does this but it should be part of Craft.

TonyDeStefano commented 5 years ago

@mmikkel / @theskyfloor / @ryanpcmcquen, I submitted PR #3718 to close item number 3 of the original request.

michaelrog commented 5 years ago

Fwiw, in the interim, you can use the Walk command to perform bulk actions (including re-saving) on elements. For example:

./craft walk entries elements.saveElement --section=blog ./craft walk entries elements.saveElement --section=blog --asJob

ryanpcmcquen commented 5 years ago

./craft walk entries elements.saveElement --section=blog

@michaelrog, I get Unknown command on this in Craft 3.1.x:

$ ./craft walk entries elements.saveElement --section=blog
Unknown command: walk

Did you mean "help"?
aelvan commented 5 years ago

This turned into a discussion on how to re-save all entries; which there already are solutions for.

The big issue here is forcing a re-save, no matter if it's necessary or not. Which is a task that does not scale at all.

To illustrate the problem; in the screenshot below I'm adding a lightswitch to an entry type in a site with 25 locales. And there're four entry types. Luckily, the site is still in development, so there're only about 400 entries, which is doable. Two years down the line, when there're tens of thousands of entries, not so much.

As far as I recall, the reason the re-save was added, was for url's to update if the field added was used in the section's url pattern. This seems like an extreme edge-case, and also something that could easily be checked for on save. A config setting for disabling re-save would be beneficial in any case.

image

brandonkelly commented 5 years ago

Next Craft release adds a new resave console controller for bulk-resaving elements. (a9867818a34607fa55a440c2a12f6703f7e7a011)

brandonkelly commented 5 years ago

The next 3.1 release has a new craft\services\Sections::$autoResaveEntries property, which can be set to false from config/app.php:

<?php

return [
    'components' => [
        'sections' => [
            'autoResaveEntries' => false,
        ],
    ],
];

(I didn’t feel comfortable creating a general config setting for this because it’s introducing some risk / unexpected behavior if you disable it, without really any gain most of the time.)

mmikkel commented 5 years ago

That's going to work very nicely @brandonkelly – thanks a lot!

sjelfull commented 5 years ago

@brandonkelly Re. bullet point 2, could the default behaviour be smarter?

mmikkel commented 5 years ago

Making the default behaviour smarter is definitely something I'd still hope to see in a future release.

brandonkelly commented 5 years ago

Yeah I should probably do that as well…

brandonkelly commented 5 years ago

Alright, that’s done for the next release as well.

sjelfull commented 5 years ago

@brandonkelly Could you also add events before and after a resave happens/fails?

There is many plugins including a bunch of mine that listens on element save/deletion and does something like creating a queue job for async processing.

This means we usually ends up with 10000s of queue jobs on large sections.

The way I have done this previously is listening to the queue exec events, but that doesn't work with the new resave console command.

brandonkelly commented 5 years ago

@sjelfull There isn’t actually a concept of bulk-resaving elements within the services or anything, so not sure a centralized event would make sense.

You can be notified before a resave action occurs by doing this though:

use craft\console\controllers\ResaveController;
use yii\base\ActionEvent;
use yii\base\Event;

Event::on(ResaveController::class, ResaveController::EVENT_BEFORE_ACTION, function(ActionEvent $e) {
    // ...
});
khalwat commented 5 years ago

There is many plugins including a bunch of mine that listens on element save/deletion and does something like creating a queue job for async processing.

This means we usually ends up with 10000s of queue jobs on large sections.

The way I have done this previously is listening to the queue exec events, but that doesn't work with the new resave console command.

@sjelfull I took have tasks that do similar things; what I do now is check the scenario of the Element. If it's set to SCENARIO_ESSENTIALS, then I know a resave elements task is in progress, and ignore the event (and don't trigger a re-save).

https://github.com/nystudio107/craft-seomatic/blob/v3/src/services/MetaBundles.php#L422

Not sure if that helps?

putyourlightson commented 5 years ago

I'm interested in this for Blitz and the ResaveController::EVENT_BEFORE_ACTION event looks ideal for this, don't you agree @sjelfull ?

khalwat commented 5 years ago

@putyourlightson The problem is if someone runs it via console command, you won't be notified (if that matters for you).

putyourlightson commented 5 years ago

This is a console controller though, craft\console\controllers\ResaveController...

khalwat commented 5 years ago

haha okay sorry -- well, then the reverse would be true, no? That it won't detect it when it's kicked off via the CP?

putyourlightson commented 5 years ago

Correct, this is only for detecting resaves performed through console commands. For CP actions, we're listening for Queue::EVENT_BEFORE_EXEC.

Event::on(Queue::class, Queue::EVENT_BEFORE_EXEC,
    function(ExecEvent $event) {
        if ($event->job instanceof ResaveElements) {
            ...
        }
    }
);
brandonkelly commented 5 years ago

Checking $element->scenario === Element::SCENARIO_ESSENTIALS is a good approach as well, @khalwat. That is generally only used on bulk resaves.

sjelfull commented 5 years ago

@brandonkelly This is what I'm currently doing to just handle the case of the ResaveElements job:

Event::on(Queue::class, Queue::EVENT_BEFORE_EXEC, [$this, 'onBeforeExec']);
Event::on(Queue::class, Queue::EVENT_AFTER_EXEC, [$this, 'onAfterExec']);
Event::on(Queue::class, Queue::EVENT_AFTER_ERROR, [$this, 'onAfterExec']);

// Redis handler
Event::on(RedisQueue::class, RedisQueue::EVENT_BEFORE_EXEC, [$this, 'onBeforeExec']);
Event::on(RedisQueue::class, RedisQueue::EVENT_AFTER_EXEC, [$this, 'onAfterExec']);
Event::on(RedisQueue::class, RedisQueue::EVENT_AFTER_ERROR, [$this, 'onAfterExec']);

This doesn't include the other queue classes.

In addition, I need to listen to the ResaveController::EVENT_BEFORE_ACTION (which doesn't handle errors), right?

See where I'm going with this? I'd say this is a good case for having two/three consistent events for this.

brandonkelly commented 5 years ago

Yeah I think @khalwat’s suggestion is a good one. You could just listen to Element::EVENT_BEFORE_SAVE and then check the element’s scenario. If it’s set to Element::SCENARIO_ESSENTIALS, there’s a pretty good chance it’s a bulk resave.

putyourlightson commented 5 years ago

@brandonkelly Isn't an element’s scenario set to Element::SCENARIO_ESSENTIALS whenever it is propagated across sites, even when performed in a single save? So the scenario is set to SCENARIO_LIVE for the element in the current site and SCENARIO_ESSENTIALS for all other sites.

brandonkelly commented 5 years ago

Ah yeah, that’s right, though you can detect if an element is propagating by checking $element->propagating.

I’ll go ahead and add a similar resaving property that you can check.

brandonkelly commented 5 years ago

And done. So as of 3.2.22, you will be able to do this:

Event::on(Element::class, Element::EVENT_AFTER_SAVE, function(ModelEvent $e) {
    /** @var Element $element */
    $element = $e->sender;

    if ($e->resaving) {
        return;
    }

    // ...
}
putyourlightson commented 5 years ago

This is great @brandonkelly!

(Sorry for the but...) I still don't think it quite addresses the use-case that @sjelfull and I are trying to solve with our plugins, in which we want to perform a specific action after a bulk element resave is complete. The new resaving property will help us detect when a single element is being resaved, but we'll still need to rely on Queue::EVENT_AFTER_EXEC, Queue::EVENT_AFTER_ERROR and ResaveController::EVENT_AFTER_ACTION to detect when the resave has completed. I also don't see any events being triggered after an asset index action (neither the CP nor console actions), which is a kind of bulk element resave.

I understand if this is asking a lot and I have an idea for how to solve the use-case in the plugin itself, in case this is something you do not plan on addressing.

brandonkelly commented 5 years ago

Ah gotcha, that’s a fair point. I just added a new craft\services\Elements::resaveElements() method for v3.2, with before/after events wrapping the entire method as well as each individual element, and now the ResaveElements job and resave/* console actions now go through that service.

putyourlightson commented 5 years ago

This looks like the best possible outcome, thanks so much @brandonkelly!

jamesmacwhite commented 5 years ago

Sorry to comment on a closed issue, but I wanted to clarify, if I was to add a new field to one or more entry types programmatically i.e. through a content migration, would the resaving behaviour change now mean the entries would not be saved by default after the new field was added?

brandonkelly commented 5 years ago

@jamesmacwhite Correct, as of c1392c94a4e0583fc6c9ac3316065a4216add22b (Craft 3.1.21+) entries will no longer be resaved after changes are made to a field layout, whether that change originated in the Control Panel or a content migration.

Worth mentioning that if you are planning on making entry type changes via content migrations, you should ensure your useProjectConfigFile config setting is disabled, per this caveat in the project config docs.

jamesmacwhite commented 5 years ago

@brandonkelly Ah, thanks for this. This explains a recent issue I've noticed then. Given this change how can I force the entries to be saved programmatically when saving a new field to a layout it belongs to? Given I have a content migration which already grabs specific sections/layouts for adding a new field?

The entries need to be saved for the field data to be populated in the Craft content DB table otherwise they will all be null.

brandonkelly commented 5 years ago

Which field type(s) are you thinking of? The only built-in field type that would add a value to existing entries after being added to an entry type is Lightswitch, which will set its values to '0' (MySQL) or false (Postgres), which is relatively inconsequential.

In any case, as of Craft 3.1.15 you can run the following command to manually resave entries in a section & of a certain entry type:

./craft resave/entries --section=mySectionHandle --type=myEntryTypeHandle
jamesmacwhite commented 5 years ago

@brandonkelly I don't want to derail the original purpose of this issue, so I'll try and be concise as possible! In a content migration I'm create a new field and adding it to a specific set of field layouts, by using a multiple level loop, section, entry types, field layout, tabs. Works fine, field gets applied to the various places. The issue is the new field itself, in this case it's the SEO Settings field from SEOMatic (replacing the previous Craft 2 SEOMatic meta field) is null and until the entry is saved it stays this way.

I see the ResaveController provides this functionality, but as this is being done in a migration, resaving the entries here will surely hit the max execution time as it will take too long to get through the amount of entries, even with limiting the criteria. I wasn't sure if can you run Craft CLI commands in a migration, but in this case I don't think I'd want to, given the timeout issue. It would seem it would be better to off-load as a queue job I guess.

brandonkelly commented 5 years ago

Ah, didn’t think about SEOmatic. I suppose that’s a good argument for auto-resaving entries whenever a new field is added to an entry type’s field layout. I’ll fix this in core so your migration doesn’t have to worry about it.

jamesmacwhite commented 5 years ago

@brandonkelly You hero. Amazing! Thanks Brandon!

brandonkelly commented 5 years ago

Fixed for today’s release!

jamesmacwhite commented 5 years ago

Not all heroes wear capes! Many thanks for this!

aelvan commented 5 years ago

Does this mean that we're back to the original behavior where any changes to entry types results in a full resave?

If so, I question the priorities that led to this decision.

khalwat commented 5 years ago

(the following is entirely tangental to the general issue here, so feel free to hit me up on Discord if you have any questions on it @jamesmacwhite ).

FWIW @jamesmacwhite most of the time using SEOmatic for Craft 3, you do not need to use a Field at all... and instead just set up mappings as appropriate via Content SEO.

When you upgrade from Craft 2.x to Craft 3.x, SEOmatic will migrate your old SEOmatic Meta field settings to the new Content SEO settings.

brandonkelly commented 5 years ago

@aelvan Only when a field is added or removed. If you just change the position of a field, or rename a tab, etc., it will still not trigger a resave.

mmikkel commented 5 years ago

If a third party field type requires resaving entries when that fieldtype is added to (or removed from) a field layout, I think triggering that resave should be the plugin's responsibility.

This is definitely reverting to old behaviour where Craft is going to trigger a lot of unneccessary resaves, and I'm a bit puzzled by the change since none of the native fieldtypes really require it (apart from Lightswitch as you mention @brandonkelly – but that is easily dealt with).

aelvan commented 5 years ago

@brandonkelly Yes, the original issue had to do with adding fields to an entry type, so what I outlined in https://github.com/craftcms/cms/issues/3482#issuecomment-465105325 is back after todays release then.

I'm really bummed about this. I see this as a fundamental issue that impacts every Craft site of some size (content-wise). When working with active clients over time (which is what we've learnt is the smart ting to do), we regularly add and remove fields from entry types as their needs evolve over time. The current behaviour does not work at scale. I get that we can now disable autoResaveEntries in app.php and run the resave console command (if we actually have access to do that on the client's server) if needed, but... this is so that someone can write a content migration that adds a field and gets it auto-populated from a different field because the plugin in question changed field types from one version to another? Seems like an extreme edge-case to me. Wouldn't it make more sense to just run the console command after the content migration? Or let the plugin figure out a solution for this (like, a migration tool you can run separately or something)?

khalwat commented 5 years ago

FWIW, I'd tend to agree with @mmikkel and @aelvan on this one.

To be clear, SEOmatic doesn't require a resave when its field type is added in the Craft 3 version. It works via Content SEO for the SEO mappings in most cases.

My memory is a bit dim, but I think the Craft 2 version did, but I always just told people to re-save the section when they added a field (which then triggered the re-save entries task).

brandonkelly commented 5 years ago

Thought this over and agree as well. Field types aren’t notified when they’re added/removed from a field layout, but that’s something we should change. And field types’ normalizeValue() methods can handle a null value, whether on a bulk resave or on a typical request.

jamesmacwhite commented 5 years ago

Without hijacking this thread again, is the only way to resave entries attached to a changed entry type through the ResaveController then?

mmikkel commented 5 years ago

@jamesmacwhite You could probably create ResaveElements queue jobs from your content migration, similar to how Craft does it?