Posted by Damien Tournoud on September 14, 2012 at 11:10am
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | dblog.module |
| Category: | bug report |
| Priority: | major |
| Assigned: | Fabianx |
| Status: | needs work |
| Issue tags: | needs backport to D7, Needs tests |
Issue Summary
Problem/Motivation
It's completely unacceptable to break a page if we fail to log a message. In particular, this cause painful to debug errors during the installation process, as recently reported in #429188: Watchdog table not installed when watchdog module enabled..
Proposed resolution
Catch and ignore all exceptions thrown in dblog_watchdog().
Remaining tasks
- Write a patch.
- Implement a generic test that tries to watchdog() in a Unit Test Case (that doesn't have access to the database).
User interface changes
None.
API changes
None.
Comments
#1
Changing issue title and bumping priority, because:
a) This happens on D7 and D8 with MySQL.
b) The module is enabled, but watchdog table does not exist, such leading to more errors.
Reproducing steps:
* Have any module that adds a message to watchdog on every page load (in this case .../modules/contrib/date/date_context/date_context_date_condition.class.php)
* Enable watchdog either during installation or afterwards.
* Watchdog _is_ enabled, but the watchdog table is missing and leads to more errors on every page load.
IMHO this is at least major, if not critical.
#2
Adding tags
#3
actually it should be the caller of hook_watchdog who eats up the exceptions not each module.
#4
Lets see what testbot says to this patch / test.
( Patch does not include feedback from #3 due to x-post)
#5
#6
The last submitted patch, core-Fix-problem-with-watchdog-exceptions-1784548-3.diff, failed testing.
#7
#8
#9
The last submitted patch, core-Fix-problem-with-watchdog-exceptions-1784548-7.diff, failed testing.
#10
Please add a @throws directive to the docblock for dblog_watchdog() when this patch is next re-rolled.
#11
#12
Re-roll that fixes the path to install.inc and adds a docblock to the test method.
@Lars Toomre: The purpose of this patch is to *prevent* that an exception is thrown, not throw one, so it's not necessary to do that. We'd actually have to remove it if there would be one.
@chx: I think it's the responsibility of each module to handle this. Doing it in watchdog() would require is to try/catch each call separately so that each hook is invoked, even if one fails.
#13
The last submitted patch, dblog-catch-exception-1784548-12.patch, failed testing.
#14
Hm. I don't really get why we need that second part of the test? It's complicated due to static caches and not what this patch is about?
Here's a version without that part...
#15
#16
The last submitted patch, dblog-catch-exception-1784548-14.patch, failed testing.
#17
I have a second patch, which might be better (based on chx feedback), but which needs work.
Just posting as I had worked on it already. Thanks @berdir:
Please note is a multi patch due to time constraints ...
#18
#19
The last submitted patch, core-Fix-problem-with-watchdog-exceptions-1784548-17.diff, failed testing.
#20
#14: dblog-catch-exception-1784548-14.patch queued for re-testing.
#21
#22
What about trying to create the table inside the catch? Eventually watchdog should be a class, and that class should be responsible for installing its own schema, similar to #1167144: Change notice: Make cache backends responsible for their own storage (note my opinion isn't necessarily that popular in that issue either).
#23
I want to add an observation that appears to be related to this issue.
When these symptoms are present
drush pmlshows dblog as not installed butadmin/modulesshows it as installed. Adrush en dblog -yalso states that dblog is already installed.Update:
Drupal 7.21
Drush 5.1
I also stated above initially that
drush en dblog -ystates dblog is not installed, I meant to say it states it is already installed and have corrected it above.#24
Btw, I would support this as critical too. I cannot flush cache on a site affected by this right now. See screenshot.
#25
Drush reports as 'not installed' because it checks that the schema version whereas the GUI checks the status.