FIX - limit number of embedded media items in a post (PR #10391)

GitHub

We can’t rename plugins in migrations because this might break existing plugins that are using the site settings. Instead we have to deprecate it first using SiteSetting::DeprecatedSettings. In the migration we will simply copy the value of the old site setting to the new site setting. Another thing that we need to consider is that the migration is going to run before the web containers are deployed with the new code. Therefore, there is technically a window where the user may have changed the site setting after we’ve already copied the value to the new role. However, we usually don’t care about that because the odds of someone changing a site setting while we’re deploying is really low.

I searched through all known plugins and themes and none mentioned newuser_max_images - I think this is safe to change.

There could be private plugins that are not known. Even if we don’t deprecate the site setting, we can’t rename site settings directly on our deployment process.

I’m not concerned about plugins that are beyond our ability to track, especially if they are dealing with with such core functionality which would be risky to change anyway. I also think the risk of someone changing a site setting during a deploy is too low to seriously care about. I think this approach is fine.

@jbrw I’ll leave it up to you. If you prefer to deprecate go for it, otherwise this is fine to merge.

Therefore, there is technically a window where the user may have changed the site setting after we’ve already copied the value to the new role. However, we usually don’t care about that because the odds of someone changing a site setting while we’re deploying is really low.

Someone changing a site setting is fine and is what I said initially as well.

The problem here is that the migration will run before the new application code has been deployed. That means the site setting override for newuser_max_images is reverted back to the default while the application code is still referencing SiteSetting.newuser_max_images.

@tgxworld I’ve just pushed a commit to add the two new values to DeprecatedSettings.

As far as I can see, I’m doing the rename of the setting in the same style that other similar renames were done in the past. Is there an example of a site setting rename that was done in a different (more comprehensive) style?

Can you explain to me how the site setting revert back to default? Site settings are cached in ruby’s memory while processes are running and aren’t refreshed unless they receive a message bus notification or the process restarts. After the migration the processes should be cycled and the new version will be present.

Site settings are cached in ruby’s memory while processes are running and aren’t refreshed unless they receive a message bus notification or the process restarts.

Sorry I totally forgot that site settings are cached in memory which means you’re right that there is no need to copy the row in the normal migration and then delete the old row in a post migration.

cc @lis2 Looks like I gave you the wrong advice previously :sweat:

No worries.