-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Check if remote storage is enabled before saving local file #2627
Check if remote storage is enabled before saving local file #2627
Conversation
Doesnt fix it for me. Config is set to DB w/o existing table ...
Sample data: https://openmage.ddev.site/men/new-arrivals/linen-blazer.html
|
I even cant switch to "file" from admin config ...
.... Btw ... "synchronization" to DB means images get deleted ... When you test this PR/issue, make sure to have a backup of your media dir ... :( |
This PR was only meant to fix the error when the config is set to file, but that might be a good thing to include as well. |
How did you end up with file storage set to Database without the table being created already??? File System is the default and when I switch to Database, the table is properly created. |
I've changed the value in core_config_data, but this could also happen with tools that import/export config. |
1fccd36
to
79cbbc5
Compare
I'm not a fan of the second commit as the storage backend isn't intended to be updated manually from the database, but here it is. It will initialize the database storage every time it's loaded which will create the table if it doesn't exist. The second commit needs to be analyzed to ensure that it won't affect performance though. |
at this point why don't we add an update script that creates the table? |
There is already one but it's conditioned by |
if we've to check it every "request" I'd prefer to have the table created and that's it |
Setting config values directly in the database, while it works in most cases, bypasses the before/after save methods of the backend models so I think should be viewed as an "unsupported" action.. So in this case the fix is to change the storage method via the admin UI so the hooks can run as intended. Import/export tools should ideally use the models to update the config rather than manipulating the db directly. |
This reverts commit 79cbbc5.
I reverted the second change to keep this PR in scope of the linked issue, which is what it was supposed to solve in the first place. Anything else can be addressed in a following PR. |
Description (*)
This PR will add a simple check if remote DB storage for media is enabled before trying to write the file locally (using
get.php
). This will prevent unnecessary exceptions from being logged that saycore_file_storage
doesn't exist, since it's often created at runtime.Fixed Issues (if relevant)
Manual testing scenarios (*)
http://<magento_host>/media/catalog/notfound.jpg
. You will see an exception logged before this patch, but not after.Contribution checklist (*)