Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mmikkel opened this issue Nov 21, 2018 · 51 comments
Labels
enhancement improvements to existing features system administration 💼 features related to system administration

Comments

@mmikkel
Copy link
Contributor

mmikkel commented Nov 21, 2018

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

  • Craft version: 3.0.31
@brandonkelly
Copy link
Member

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.

@brandonkelly brandonkelly added enhancement improvements to existing features system administration 💼 features related to system administration labels Nov 21, 2018
@mmikkel
Copy link
Contributor Author

mmikkel commented Nov 23, 2018

@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
Copy link

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
Copy link

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
Copy link
Contributor

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

@michaelrog
Copy link

michaelrog commented Jan 25, 2019

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
Copy link

./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
Copy link

aelvan commented Feb 19, 2019

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
Copy link
Member

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

@brandonkelly
Copy link
Member

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.)

@brandonkelly brandonkelly removed this from the 3.2 milestone Mar 28, 2019
@mmikkel
Copy link
Contributor Author

mmikkel commented Mar 28, 2019

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

@sjelfull
Copy link
Contributor

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

@mmikkel
Copy link
Contributor Author

mmikkel commented Mar 29, 2019

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

@brandonkelly
Copy link
Member

Yeah I should probably do that as well…

@brandonkelly
Copy link
Member

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

@sjelfull
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

khalwat commented Mar 31, 2019

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
Copy link

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

@khalwat
Copy link
Contributor

khalwat commented Mar 31, 2019

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

@putyourlightson
Copy link

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

@khalwat
Copy link
Contributor

khalwat commented Mar 31, 2019

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?

@brandonkelly brandonkelly reopened this Apr 8, 2019
brandonkelly added a commit that referenced this issue Apr 8, 2019
resolves #3482

Signed-off-by: brandonkelly <brandon@pixelandtonic.com>
@brandonkelly
Copy link
Member

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
Copy link

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

@jamesmacwhite
Copy link
Contributor

jamesmacwhite commented Apr 22, 2019

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
Copy link
Member

@jamesmacwhite Correct, as of c1392c9 (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
Copy link
Contributor

jamesmacwhite commented Apr 23, 2019

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Contributor

@brandonkelly You hero. Amazing! Thanks Brandon!

@brandonkelly
Copy link
Member

Fixed for today’s release!

@jamesmacwhite
Copy link
Contributor

jamesmacwhite commented Apr 23, 2019

Not all heroes wear capes! Many thanks for this!

@aelvan
Copy link

aelvan commented Apr 23, 2019

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
Copy link
Contributor

khalwat commented Apr 23, 2019

(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
Copy link
Member

@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
Copy link
Contributor Author

mmikkel commented Apr 23, 2019

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
Copy link

aelvan commented Apr 23, 2019

@brandonkelly Yes, the original issue had to do with adding fields to an entry type, so what I outlined in #3482 (comment) 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
Copy link
Contributor

khalwat commented Apr 23, 2019

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
Copy link
Member

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.

brandonkelly added a commit that referenced this issue Apr 23, 2019
@jamesmacwhite
Copy link
Contributor

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

@mmikkel
Copy link
Contributor Author

mmikkel commented Apr 23, 2019

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

@jamesmacwhite
Copy link
Contributor

@mmikkel That would seem to be the best option from everything mentioned. I'll look at doing that. Thanks for advice.

Sorry for adding more fuel to the fire here! It was not my intention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features system administration 💼 features related to system administration
Projects
None yet
Development

No branches or pull requests

10 participants