backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Config value `file_default_allowed_extensions` in `system.core.json` seems unused #6298

Open docwilmot opened 10 months ago

docwilmot commented 10 months ago

Description of the bug

Not a bug per se, but that doesnt appear anywhere else in core. We should remove it if its unused.

Additional information

Add any other information that could help, such as:

indigoxela commented 10 months ago

This ghost config item is very confusing, so let's look at the history first:

file_default_allowed_extensions came in back in 2018 with Issue #2633 At that time this setting in system.core.json has been used in function file_get_upload_validators.

But then in 2019 there was a switch to default_allowed_extensions in file.settings.json, which replaced the formerly used config item in file_get_upload_validators() and other places. Since then the old config item from system.core.json isn't in use anymore, but it hasn't been removed.

That was issue #2632

I wonder, if it's possible to remove the old config item. There's no admin UI for it, on /admin/structure/file-types/settings we set default_allowed_extensions.

avpaderno commented 10 months ago

It does not seem the file_default_allowed_extensions value has been moved to default_allowed_extensions.
I was going to say that could eventually be something to do, but that should be avoided if the file_default_allowed_extensions value is different from the current value of default_allowed_extensions or from the default value for default_allowed_extensions. (In other words, in the only cases it should be done, the values for default_allowed_extensions and file_default_allowed_extensions are the same.)

docwilmot commented 9 months ago

It does not seem the file_default_allowed_extensions value has been moved to default_allowed_extensions. I was going to say that could eventually be something to do, but that should be avoided if the file_default_allowed_extensions value is different from the current value of default_allowed_extensions or from the default value for default_allowed_extensions. (In other words, in the only cases it should be done, the values for default_allowed_extensions and file_default_allowed_extensions are the same.)

I dont quite understand your meaning here. I think we just need to delete that from system.core.json.

docwilmot commented 4 months ago

Bump. Can we delete this?

indigoxela commented 4 months ago

@docwilmot I saw that you simply removed the item from default system.core.json, but didn't implement an update hook to remove from config in existing sites. That's by intention, I guess?

avpaderno commented 4 months ago

What I meant in my previous comment is that file_default_allowed_extensions has not been migrated to the new configuration value and that doing it now is too late. At this time, file_default_allowed_extensions can only be deleted.

docwilmot commented 4 months ago

I dont think I'm meant to add this to 1.28. Removing.

docwilmot commented 4 months ago

@docwilmot I saw that you simply removed the item from default system.core.json, but didn't implement an update hook to remove from config in existing sites. That's by intention, I guess?

I dont think its used at all but I suppose its best not to delete config from existing sites still? Doesnt harm to leave it there I suspect.