In Drupal 7, we switched the watchdog system to use error levels consistent with RFC 3164. This is a good thing. It's such a good thing, in fact, that PHP itself did so as well a long time ago. The watchdog constants we defined already exist in PHP!
Since there's talk in other issues of the supposed performance cost of constants, here's an easy way for us to eliminate 7 completely pointless constants by just using what PHP already provides.
Comment | File | Size | Author |
---|---|---|---|
#69 | logging_severity_levels-1136130-69.patch | 6.49 KB | pillarsdotnet |
#64 | watchdog.patch | 69.66 KB | catch |
#63 | patchxx.patch | 93.11 KB | keichee |
#61 | watchdog_constants-1136130-61-revert.patch | 76.13 KB | pillarsdotnet |
#61 | watchdog_constants-1136130-61-followup-1-d8.patch | 7.79 KB | pillarsdotnet |
Comments
Comment #1
Crell CreditAttribution: Crell commentedAnd patch. (I just needed an issue number.)
This is my first Drupal 8 patch, and it's an API breaker. Go me. :-)
Another advantage here: One less Drupalism, and one more thing familiar to existing PHP developers.
Comment #2
Dries CreditAttribution: Dries commented154 insertions, 242 deletions = I like! :)
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commented+1
Comment #4
jbrown CreditAttribution: jbrown commentedNice find @Crell !
Is @Dries allowed to RTBC?? ;-)
+1 from me too
Comment #5
Crell CreditAttribution: Crell commentedDries: Oh great, if you RTBCed it then who can commit it??? Quick, someone call Steven Wittens to do it!
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedAlthough this is a great patch, I hope that it does not get applied -- until at a minimum, it also includes D8 documentation and an entry for what will become the D7 to D8 upgrade guide. As Crell points out in #1, this is an API breaker from D7.
This patch is in essence a variable/constant rename patch. To facilitate the ease of generating patches for both D7 and D8, I would encourage that this patch also include the definitions of the current WATCHDOG_* constants as well as renaming whatever of those constants now are in core. That way patches primarily geared to fix D7 issues will still work against D8 head. A separate release blocker issue could then be opened that included a patch that removed these D7 WATCHDOG_* definitions and hence it would catch any additional WATCHDOG_* definitions that might appear.
I also would suggest that this change include a D7/D6 documentation patch that explains that the WATCHDOG_* constants will be eliminated in D8 and the PHP constants can be used in D7 (and D6) as is. That way contrib modules have a heads up to start changing their code to use the preferred constants.
Comment #7
jbrown CreditAttribution: jbrown commentedLars: This isn't how core development works. Commits are made to D8 so that it is the way we want it to be. The more D8 diverges from D7, the harder it is to backport patches. This is the price we pay.
This patch is very minor compared to what is coming. Trying to maintain patch compatibility is madness / not possible.
It is standard practice for API changes to be documented at http://drupal.org/update/modules . This doesn't get included in the patch.
Comment #8
Crell CreditAttribution: Crell commentedInline documentation has already been updated in the patch. For D8, no other internal documentation is necessary.
For the upgrade guide, standard procedure is that we don't add an item to the upgrade guide until the patch is actually applied. There is talk about a new method for handling update reports using nodes, but AFAIK that is not yet in place. If/when this is committed I will create a new update page accordingly and document it using the current standard.
Changing the API of Drupal 7 is way out of scope for this issue. We cannot break the existing API by removing those constants. We could switch Drupal 7 core to use the PHP constants instead internally and encourage people to use them, but that's better done via a blog post or something than via this issue, IMO. That's also webchick's call, not mine.
This issue is not about "fixing D7". Please let us stay on topic.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedWell, I will shut up then and not bother with any patches that might include watchdog() calls. Sorry for interrupting your topic.
There is a certain arrogance in both replies to my suggestion above. Why bother to attempt to correct known problems identified in D7 if that patch has to be applied to D8 first using undocumented API changes? And then changed back to use the D7 constants if and when the D8 version is finally applied? Color me as one whose first reaction is why bother.
I thought Drupal wanted to grow its community. I am shaking my head in disgust. I am truly sorry for bothering to even have spoken up.
Comment #10
Dries CreditAttribution: Dries commentedThe reason I didn't commit this patch yet is because I wanted to provide some more time/room for discussion.
Comment #11
Crell CreditAttribution: Crell commentedLars: Really, you're making a mountain out of a molehill. What exactly is "undocumented" about this patch? Look at the patch; the inline API documentation is updated. There is no D7->D8 update guide yet, but when there is (which may be created as a result of this patch) I pledge that I will document this change as appropriate, using the method Drupal currently uses for documenting API changes.
We cannot remove the existing constants from Drupal 7, as that's an API break. We don't break APIs in a stable version. Does it make sense to deprecate them and use the PHP native constants in core instead? Maybe. There would be zero performance difference from doing so, though, since we cannot remove the constants. Whether or not it's worthwhile for DX is an open question.
If you think we should do that, the correct way to do so is to volunteer to write the backport patch. (It's really simple to do for a patch like this at this point.) Whether or not that would be accepted is up to webchick, who is the D7 maintainer.
We insist on fixing issues in the dev version first before stable for the simple reason that we want to avoid regressions. If we fix an issue in D7 but then never quite get around to it for D8, bam, we've just screwed developers over even further.
Whining that we need to completely change the way we manage backports for the sake of a few constants is not going to get you anywhere. There are other threads discussing changes to how we handle backports. If you have constructive suggestions on that front, please go add them in #1050616: Figure out backport workflow from Drupal 8 to Drupal 7.
Comment #12
Crell CreditAttribution: Crell commentedAdding my new favorite tag. :-)
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commented@Crell - I am not trying to make a mountain out of a molehill. Nor am I trying to change the back-port process; that certainly is not my intent. As I said in #6, this is a good patch and should be applied before D8 is released. My issue is one of timing and documentation.
There are quite a number of D7 patches that first need to be applied against D8 and then against D7. In many cases, they are the same patch since the code bases have not diverged too much yet. This patch renames the constants for a fairly popular function watchdog(). Hence, two patches would now need to be produced - one each with D7 and D8 constants, but no other differences.
Perhaps my explanation was inelegant above. My point was why not keep everyone relatively productive during the initial period of the D8 development cycle? Why not hold off on applying this patch (which is a good patch!!) until at least there has been a 7.1 release and many of the backed-up patches have been applied/resolved?
My thought about the documentation was that we should alert developers that the watchdog constants are being eliminated and even now can be replaced in D6/D7. Your comments in the patch itself are fine. Many developers (like myself) generally reference api.drupal.org when checking for constant definitions. It was that piece of the documentation that I hope would be applied together with this patch.
I hope that this explanation helps. I am sorry if offended in any way.
Comment #14
Crell CreditAttribution: Crell commentedI'm fine with adding "this is deprecated" documentation to Drupal 7's watchdog constants, but that would by nature have to be a separate patch from the one in #1 above (which is for Drupal 8). If you wanted to make such a patch (probably a separate thread from this one) I would support it.
Comment #15
Dries CreditAttribution: Dries commentedCommitted the patch to 8.x. Thanks Crell et al.
I'm marking this 'needs work' so we can create the 'this is deprecated' patch per #14.
I noticed the following when committing the patch to 8.x:
mode change 100644 => 100755 sites/default/default.settings.php
I undid that change prior to committing to pushing this patch. I believe this that was the right thing to do.
Comment #16
andypostSubscribe
Comment #17
tstoecklerCrell in #8:
Moving back to Drupal 8 and adding the "Needs Update Documentation" (why is that CamelCase?) tag.
Comment #18
webchickDries, did you mean for this change to get backported to D7? If so, we'd need to re-roll the patch to not remove the existing constants, as that would be a tremendous backwards compatibility breakage.
We *could* backport this to D7, I guess, in the interest of making the upgrade path slightly smoother since people could start renaming their constants now but nothing would break if they didn't. I don't see a hugely compelling reason to do so, though, and don't like how many hunks it changes for such a simple pedantic thing (to be clear, I'm all about cleaning up pedantic things like this in D8, but would rather focus on fixes for D7 that have a direct impact in bug freeness and stability).
Comment #19
catchI broadly agree with Lars here. I completely support API changes between major versions, fixing in the development release first and backporting etc. however this only works if patches which actually need to be backported get committed promptly to Drupal 7, and this has not been happening since release.
At Dries' Chicago keynote, he stated that Drupal 8 would not be allowed to have more than 15 critical issues at any one time. When it got past that point, some kind of action would be taken, for example not committing large new features, or API changes.
There are currently 19 critical issues against Drupal 8, and 2 against Drupal 7, way past that limit.
Not only this, more than 15 of the Drupal 8 critical issues need to be backported to Drupal 7
Three of those are RTBC, and also were when this patch didn't even exist.
Yet this patch, which will make it harder to fix Drupal 7 issues, gets committed with hardly any community review within a couple of weeks. The patch itself is fine, the fact the RTBC queue is FIFO is fucked.
The intention of the 15 criticals idea at the keynote was to keep the development release relatively stable at all times so that we're never too far from a release, right now it's not even allowing critical issues in the stable release to be fixed promptly.
Comment #20
Crell CreditAttribution: Crell commented@Dries: Yeah, git really likes to mess with file permissions. I'm not entirely sure how make it stop that.
@webchick: I don't think anyone is suggesting that we remove the existing watchdog constants in Drupal 7, since as you said that would be a BC breakage. I think we're all on the same page that we should document in Drupal 7 that the native constants work just fine and you should be using those instead since the watchdog constants are going away.
What's up for debate is whether we also switch Drupal 7 core to use the native constants instead of the watchdog constants so that people who just copy from core will get the native constants. Given the points that Lars and catch made above regarding how many other patches that would break, I'm inclined to say no myself. A small documentation patch is enough.
I will roll a "deprecated" patch for D7 as soon as I get a chance.
Comment #21
catchActually any existing D7 patches are already invalidated by this in that they'd need to be re-rolled for 8 if they previously applied to both branches. Right now there is not that much divergence and the constants could be the only change. So there is no cost to existing patches in the queue if we committed a similar patch to 7, as long as that happens quickly before any affected patches have already been rerolled. Once it's in, it would to an extent reverse the divergence that was introduced (IMO prematurely given the very bad state of D7 and inactivity in other parts of the rtbc queue). Having this discussion before any commits had been made would have made so much more sense here.
Comment #22
tstoecklerI added this issue to the update documentation.
Comment #23
wmostrey CreditAttribution: wmostrey commentedComment #24
catchDrush 8.x support needs an update for this #1155354: Update 8.x support for watchdog constants.
Comment #25
jbrown CreditAttribution: jbrown commentedI don't think it's a good idea to deprecate the WATCHDOG_* constants in D7. It would be confusing to have two constants being used for the same purpose.
Only using WATCHDOG_* in D7 and only LOG_* in D8 keeps things simple.
@dries if you haven't pushed yet, use can combine the staging area into previous commit like this:
Comment #26
salvisThe original assertion at the top ...
... is not quite correct: PHP's numeric values don't follow RFC3164. For example, WATCHDOG_WARNING is 4, but LOG_WARNING is 5, at least on PHP 5.2.9.
Comment #27
jbrown CreditAttribution: jbrown commentedHere are the constants from http://www.ietf.org/rfc/rfc3164.txt
With PHP 5.3.5 on Ubuntu, running
gives
- so that's correct.
I noticed the following in win32/syslog.h in the PHP 5.3.6 source code:
Presumably it's some sort of compatibility issue with win32.
Does this have any ramifications for Drupal?
Comment #28
Crell CreditAttribution: Crell commentedYes, the ramification is that Windows users are running a crippled version of PHP due to limitations of Windows. I don't know that we can do anything about that.
(OK, that was snarky, but I think this is an area where I'd rather let the OS do its thing than try to second guess the OS.)
Comment #29
salvisYes, I'm developing on Windows.
It looks like an issue of the Windows implementation of PHP, but we should still be able to take a DB from a Linux server to a Windows box and see the same log messages.
I guess that's why we had our own constants, and they were a portability asset rather than a duplicate of existing functionality...
Comment #30
sunDrupal's logging is not supposed to be constrained to the operating system's logging facility. The fundamental constraint may exist when running syslog.module on Windows (since that uses syslog() to log to Windows' event log), but it doesn't exist for the standard dblog.module and potentially others. This means I can no longer log debugging messages, and filtering by messages by severity in dblog is broken.
Although the idea was nice, we need to revert this patch.
Comment #31
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch to revert.
Comment #33
Crell CreditAttribution: Crell commentedIf we do roll this back, the rollback needs to include a note that the only reason we're redefining these constants is because Windows is broken. Preferably a link to something on php.net about it.
Personally I would still rather just document that Windows hosts are get to deal with a broken OS.
Comment #34
Dave ReidHave to say it: f$@king windows...
Comment #35
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch to revert, including relevant comment and link to php.net.
Comment #36
catchCould we add a @defgroup to the constants? Would save duplicating the comment for each define.
Comment #37
tstoecklertypo: "constatns" -> "constants"
Comment #38
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrected spelling error and created @defgroup logging_severity_levels. Also added
watchdog_severity_levels()
to the group.Comment #39
catchLooks good to me. While I didn't agree with this patch being committed when it was (and it did end up holding up a critical issue last night for about 20 minutes dealing with patch fuzz at about 2am for webchick), it was a good idea apart from that.
However lots of people develop on windows locally, and there's a chance this could be switched again later if and when the PHP bug report is fixed.
Comment #41
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe PHP bug report is a "wontfix".
Comment #42
Crell CreditAttribution: Crell commentedPHP itself is a won't fix, given the number of legit bugs they mark "bogus".
Comment #43
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded
drupal_error_levels()
to the new group.Comment #44
pillarsdotnet CreditAttribution: pillarsdotnet commentedProbably the doc changes should be backported to 7.x, if accepted.
Comment #45
pillarsdotnet CreditAttribution: pillarsdotnet commentedEDIT: ignore this.
Comment #46
pillarsdotnet CreditAttribution: pillarsdotnet commentedDivided #43 into two patches; one to reverse the change, and another to also add docs.
Backported the docs change to 7.x.
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commented#46: watchdog_constants-revert_only-1136130-46.patch queued for re-testing.
Comment #48
sunLet's do the straight rollback first and clarify docs in a second follow-up patch.
Comment #49
sun#46: watchdog_constants-revert_only-1136130-46.patch queued for re-testing.
Comment #50
webchickI don't see anything in here that needs backport to D7. I think it was going to be documentation to point out that these constants should be used, but now we're reneging on that.
Since that makes it not a D7 bug fix, and since Dries committed this in the first place, I'll leave for him. But I think the rollback makes sense. Constants that are not actually constant == FAIL.
Comment #51
pillarsdotnet CreditAttribution: pillarsdotnet commentedMoved the d7 docs change to #1196068: Document why WATCHDOG_* constants are used instead of built-in PHP LOG_* constants.EDIT: Marked that issue a dup of this one, after some discussion.
Comment #52
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-posting patches for the sake of clarity.
The first patch is the one under review; the second and third are follow-ups to be backported to D7.
Comment #53
pillarsdotnet CreditAttribution: pillarsdotnet commentedThat second follow-up patch was malformed. Correcting:
Comment #54
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #55
catchhttp://www.php.net/manual/en/function.runkit-constant-redefine.php just sayin'.
Comment #56
Crell CreditAttribution: Crell commentedAs much as I would love to tell Windows users that they have to use runkit because PHP on Window is broken, I don't know that we could get away with that.
Comment #57
Alan D. CreditAttribution: Alan D. commented@catch: lol >> in regards to #55 "A DLL for this PECL extension is currently unavailable."
Comment #58
webchickOk, even though this is not a D7 bug fix, I'd like to go ahead and commit it to D8. Further research showed this to be incompatible with Windows, so it's a no-go. And additionally, it's causing a lot of headaches trying to fix critical bugs like #1204658: Always use query metadata to specify the node access base table due to the constant changes.
However, it no longer applies. :( Sorry, that was probably my fault. I looked at the "needs backport" RTBC queue before this one.
Can I get a quick re-roll (file.inc seems problematic)?
Comment #59
Garrett Albright CreditAttribution: Garrett Albright commentedReroll.
Comment #60
keichee CreditAttribution: keichee commentedthis makes ubercart and majority of modules to comply with this.. as they are all broken, specially ubercart that uses
Comment #61
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-uploading so that testbot can verify that it applies:
watchdog_constants-1136130-61-revert.patch
LOG_*
toWATCHDOG_*
.watchdog_constants-1136130-61-followup-1-d8.patch
LOG_*
constants, as suggested by Crell in #33.watchdog_constants-1136130-61-followup-2-d8.patch
@docgroup
, as suggested by catch in #36.Comment #62
Crell CreditAttribution: Crell commentedI don't see why patches 2 and 3 are separate. Just have a single patch that fixes up the documentation once the original is rolled back.
Comment #63
keichee CreditAttribution: keichee commentedattached, as per request
Comment #64
catchRolled the 'followup-2' patch into #63 so we just add the defgroup. Hopefully this one's ready.
Comment #65
pillarsdotnet CreditAttribution: pillarsdotnet commentedYou could have cat'd the three patches in #61 together and they'd apply just as well.
Also the follow-ups are separate because they apply to D7. That's why I called them "follow-ups".
Comment #66
catchStacked patches are really hard to review by humans even if git can handle them.
While I'm the last patch author here all I did was re-roll, so I'm RTBC-ing this to get it out of limbo.
Comment #67
sunFiled a change request upstream: https://bugs.php.net/bug.php?id=55129
Comment #68
webchickYAY. Thanks. And thanks sun, for filing the upstream bug.
Committed and pushed to 8.x. Now to go fix a critical bug. :)
Comment #69
pillarsdotnet CreditAttribution: pillarsdotnet commentedBackport of documentation improvements.
Explains why WATCHDOG_* constants are used instead of PHP built-in LOG_* constants, and adds documentation group for "Logging severity levels" to combine redundant documentation blocks.
Comment #70
keichee CreditAttribution: keichee commentedwow many thanks for the commit on the git!
Comment #71
catchPatch looks great. webchick indicated she didn't think there was anything to backport here earlier, but I'm not sure if that covered the docs patch as well, makes sense to me to backport docs why we have these, even if we didn't know why we did prior to this issue.
Comment #72
catchForgot status change.
Comment #73
webchickOk, cool. This will stop us from making this mistake in the future.
Committed and pushed to 7.x. Thanks!