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

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

Open
docwilmot opened this issue Nov 17, 2023 · 8 comments · May be fixed by backdrop/backdrop#4710
Open

Comments

@docwilmot
Copy link
Contributor

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:

  • Backdrop CMS version: 1.26.1
@indigoxela
Copy link
Member

indigoxela commented Nov 18, 2023

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.

@kiamlaluno
Copy link
Member

kiamlaluno commented Nov 18, 2023

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

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

Bump. Can we delete this?

@indigoxela
Copy link
Member

indigoxela commented Apr 26, 2024

@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?

@kiamlaluno
Copy link
Member

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

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

@docwilmot docwilmot removed this from the 1.28.0 milestone Apr 27, 2024
@docwilmot
Copy link
Contributor Author

docwilmot commented Apr 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants