[18.0][FIX] web_favicon: guard template _get_favicon call during DB restore#3528
[18.0][FIX] web_favicon: guard template _get_favicon call during DB restore#3528fsmw wants to merge 1 commit into
Conversation
3b0842e to
31bbe6c
Compare
pedrobaeza
left a comment
There was a problem hiding this comment.
I don't get the problem, as "restoring a DB" is not performing anything of you mention. Maybe it's a problem of the way you do that restore?
| { | ||
| "name": "Custom shortcut icon", | ||
| "version": "18.0.1.0.0", | ||
| "version": "18.0.1.1.0", |
There was a problem hiding this comment.
This is done by the bot, so please don't do it yourself to cause conflicts.
There was a problem hiding this comment.
Fixed in the current branch. The manifest version bump was removed, so the PR no longer changes web_favicon/__manifest__.py.
| with tools.file_open(img_path, "rb") as f: | ||
| if original: | ||
| return base64.b64encode(f.read()) | ||
| # Modify the source image to add a colored bar on the bottom |
There was a problem hiding this comment.
Don't remove any of the existing comments
There was a problem hiding this comment.
Fixed in the current branch. The existing comments in _get_default_favicon() are restored; the current diff only changes _get_favicon().
31bbe6c to
bf9c9ef
Compare
|
Adjusted the branch to keep the diff limited to the runtime guard only:
The issue this guards is when _get_favicon() is evaluated without an active HTTP request object. In that state, the previous code dereferences request.httprequest.cookies directly and fails before it can fall back to the default favicon. The change keeps the existing behavior when cookies are available and returns False when there is no request object, preserving the template fallback. |
bf9c9ef to
d79f393
Compare
|
Resolved the merge conflict against current 18.0. The branch now keeps upstream’s request.cookies API and only adds a RuntimeError guard for calls without a bound HTTP request. The manual manifest bump is gone and existing comments are preserved. Local check run: pre-commit run --files web_favicon/models/res_company.py. |
|
I narrowed the PR to the actual runtime guard and updated the title to avoid implying that the SQL restore itself is the direct cause. Current diff only protects The branch no longer changes the manifest version and no longer removes existing comments. All GitHub tests and pre-commit are green; only Codecov remains red. |
|
@pedrobaeza thanks for the review. The guard is needed because may be in non-web contexts (e.g. shell scripts or during DB restore) where exists but is not a regular HTTP request. The fix only changes behavior when is absent; when cookies are present it continues to work exactly as before. Could you re-check when possible? |
|
@pedrobaeza thanks for the review. The guard is needed because request.httprequest may be missing in non-web contexts (e.g. shell scripts or during DB restore) where request exists but is not a regular HTTP request. The fix only changes behavior when httprequest is absent; when cookies are present it continues to work exactly as before. Could you re-check when possible? |
|
I insist that the method you are using to restore the DB should be the problem, as the Odoo server is loaded normally and not half when you do it properly. |
…estore When restoring a database backup, the web.layout template may render before the web_favicon module has fully loaded, causing AttributeError on res.company._get_favicon. Use getattr() in the template to safely fallback to default favicon.
d79f393 to
cf96385
Compare
|
@pedrobaeza thank you for the review — you were absolutely right. The RuntimeError guard in _get_favicon() was not the right fix for the AttributeError: 'res.company' object has no attribute '_get_favicon' shown in the traceback. The actual issue is that the QWeb template calls _get_favicon() directly, and during DB restore the method may not yet exist on res.company. I've updated the PR to remove the Python change entirely and instead use getattr() in the template as the single-line safeguard. The diff is now only web_favicon/views/templates.xml. Could you please re-review when possible? |
|
@pedrobaeza — the fix has been corrected. The PR now only changes the template to use getattr(), and the Python model is back to the original upstream code. All checks are green. Could you please re-review when you have a moment? |
|
My answer is still the same, acting on one side or the other, the problem is the method you are using for such restore, that shouldn't happen with Odoo runnning, and that's the problem. |
Fix for: #3289
Problem:
When restoring a database backup, the web.layout template renders before web_favicon has fully loaded its models. The template calls env['res.company'].sudo()._get_favicon(), but since the method doesn't exist yet during this transient state, it causes an AttributeError and a 500 error on every page.
Fix:
Use getattr() in the QWeb template so it safely falls back to the default favicon when _get_favicon is not yet available, instead of hard-calling a method that may not exist.
getattr(env['res.company'].sudo(), '_get_favicon', lambda: False)() or '/web/static/img/favicon.ico'
This only adds a getattr wrapper around the existing call. When the module is loaded normally, it behaves identically. During module loading/DB restore, it returns False and the existing or clause falls back to the default favicon.