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.

CommentFileSizeAuthor
#69 logging_severity_levels-1136130-69.patch6.49 KBpillarsdotnet
#64 watchdog.patch69.66 KBcatch
#63 patchxx.patch93.11 KBkeichee
#61 watchdog_constants-1136130-61-revert.patch76.13 KBpillarsdotnet
#61 watchdog_constants-1136130-61-followup-1-d8.patch7.79 KBpillarsdotnet
#61 watchdog_constants-1136130-61-followup-2-d8.patch9.19 KBpillarsdotnet
#59 1136130-watchdog-constants-revert-D8.patch70.2 KBGarrett Albright
#53 watchdog_constants-1136130-followup-2-d8.patch5.3 KBpillarsdotnet
#52 watchdog_constants-1136130-revert.patch76.18 KBpillarsdotnet
#52 watchdog_constants-1136130-followup-1-d8.patch7.79 KBpillarsdotnet
#52 watchdog_constants-1136130-followup-2-d8.patch17.04 KBpillarsdotnet
#46 watchdog_constants-revert_only-1136130-46.patch76.18 KBpillarsdotnet
#46 watchdog_constants-revert+docs_update-1136130-46.patch75.5 KBpillarsdotnet
#46 watchdog_constants-docs_update-1136130-46-d7.patch5.09 KBpillarsdotnet
#46 watchdog_constants-docs_update-1136130-46-d7.patch5.09 KBpillarsdotnet
#45 watchdog_constants-revert-1136130-45.patch76.83 KBpillarsdotnet
#45 watchdog_constants-docs_update-1136130-45-d7.patch3.33 KBpillarsdotnet
#43 watchdog_constants-revert-1136130-43.patch75.45 KBpillarsdotnet
#41 watchdog_constants-revert-1136130-41.patch75.31 KBpillarsdotnet
#38 watchdog_constants-revert-1136130-38.patch75.27 KBpillarsdotnet
#35 watchdog_constants-revert-1136130-35.patch78.17 KBpillarsdotnet
#31 watchdog_constants-revert-1136130-31.patch76.04 KBpillarsdotnet
#1 1136130-watchdog-constants.patch70.49 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Active » Needs review
FileSize
70.49 KB

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

Dries’s picture

Status: Needs review » Reviewed & tested by the community

154 insertions, 242 deletions = I like! :)

pillarsdotnet’s picture

+1

jbrown’s picture

Nice find @Crell !

Is @Dries allowed to RTBC?? ;-)

+1 from me too

Crell’s picture

Dries: Oh great, if you RTBCed it then who can commit it??? Quick, someone call Steven Wittens to do it!

Lars Toomre’s picture

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

jbrown’s picture

Lars: 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.

Crell’s picture

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

Lars Toomre’s picture

Well, 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.

Dries’s picture

The reason I didn't commit this patch yet is because I wanted to provide some more time/room for discussion.

Crell’s picture

Lars: 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.

Crell’s picture

Adding my new favorite tag. :-)

Lars Toomre’s picture

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

Crell’s picture

I'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.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

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

andypost’s picture

Subscribe

tstoeckler’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs documentation updates

Crell in #8:

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.

Moving back to Drupal 8 and adding the "Needs Update Documentation" (why is that CamelCase?) tag.

webchick’s picture

Dries, 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).

catch’s picture

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

Crell’s picture

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

catch’s picture

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

tstoeckler’s picture

I added this issue to the update documentation.

wmostrey’s picture

catch’s picture

Drush 8.x support needs an update for this #1155354: Update 8.x support for watchdog constants.

jbrown’s picture

I 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:

git commit --amend
salvis’s picture

The original assertion at the top ...

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!

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

jbrown’s picture

Here are the constants from http://www.ietf.org/rfc/rfc3164.txt

        Numerical         Severity
          Code

           0       Emergency: system is unusable
           1       Alert: action must be taken immediately
           2       Critical: critical conditions
           3       Error: error conditions
           4       Warning: warning conditions
           5       Notice: normal but significant condition
           6       Informational: informational messages
           7       Debug: debug-level messages

With PHP 5.3.5 on Ubuntu, running

print 'LOG_EMERG: ' . LOG_EMERG . "\n";
print 'LOG_ALERT: ' . LOG_ALERT . "\n";
print 'LOG_CRIT: ' . LOG_CRIT . "\n";
print 'LOG_ERR: ' . LOG_ERR . "\n";
print 'LOG_WARNING: ' . LOG_WARNING . "\n";
print 'LOG_NOTICE: ' . LOG_NOTICE . "\n";
print 'LOG_INFO: ' . LOG_INFO . "\n";
print 'LOG_DEBUG: ' . LOG_DEBUG . "\n";

gives

LOG_EMERG: 0
LOG_ALERT: 1
LOG_CRIT: 2
LOG_ERR: 3
LOG_WARNING: 4
LOG_NOTICE: 5
LOG_INFO: 6
LOG_DEBUG: 7

- so that's correct.

I noticed the following in win32/syslog.h in the PHP 5.3.6 source code:

#define	LOG_EMERG	1
#define	LOG_ALERT	1
#define	LOG_CRIT	1
#define	LOG_ERR		4
#define	LOG_WARNING	5
#define	LOG_NOTICE	6
#define	LOG_INFO	6
#define	LOG_DEBUG	6

Presumably it's some sort of compatibility issue with win32.

Does this have any ramifications for Drupal?

Crell’s picture

Yes, 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.)

salvis’s picture

Yes, 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...

sun’s picture

Priority: Normal » Major
Issue tags: +Regression
C:\>php -r "var_dump(LOG_EMERG, LOG_ALERT, LOG_CRIT, LOG_ERR, LOG_WARNING, LOG_NOTICE, LOG_INFO, LOG_DEBUG);"
int(1)
int(1)
int(1)
int(4)
int(5)
int(6)
int(6)
int(6)

C:\>php -r "var_dump(PHP_VERSION);"
string(5) "5.3.5"

Drupal'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.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
76.04 KB

Patch to revert.

Status: Needs review » Needs work

The last submitted patch, watchdog_constants-revert-1136130-31.patch, failed testing.

Crell’s picture

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

Dave Reid’s picture

Have to say it: f$@king windows...

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: -Proudly Found Elsewhere
FileSize
78.17 KB

Patch to revert, including relevant comment and link to php.net.

catch’s picture

Could we add a @defgroup to the constants? Would save duplicating the comment for each define.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/includes/bootstrap.inc
@@ -38,6 +38,127 @@ define('CACHE_PERMANENT', 0);
+ * PHP supplies predefined LOG_* constatns for use in the syslog() function, but

typo: "constatns" -> "constants"

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
75.27 KB

Corrected spelling error and created @defgroup logging_severity_levels. Also added watchdog_severity_levels() to the group.

catch’s picture

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

Status: Needs review » Needs work

The last submitted patch, watchdog_constants-revert-1136130-38.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
75.31 KB

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.

The PHP bug report is a "wontfix".

Crell’s picture

PHP itself is a won't fix, given the number of legit bugs they mark "bogus".

pillarsdotnet’s picture

Added drupal_error_levels() to the new group.

pillarsdotnet’s picture

Issue tags: +Needs backport to D7

Probably the doc changes should be backported to 7.x, if accepted.

pillarsdotnet’s picture

pillarsdotnet’s picture

Divided #43 into two patches; one to reverse the change, and another to also add docs.

Backported the docs change to 7.x.

pillarsdotnet’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Let's do the straight rollback first and clarify docs in a second follow-up patch.

sun’s picture

webchick’s picture

Issue tags: -Needs backport to D7

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

pillarsdotnet’s picture

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

pillarsdotnet’s picture

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

pillarsdotnet’s picture

Title: Regression: Reinstate WATCHDOG_* constants and document why they are necessary. » Don't duplicate watchdog level constants
FileSize
5.3 KB

That second follow-up patch was malformed. Correcting:

pillarsdotnet’s picture

Title: Don't duplicate watchdog level constants » Regression: Reinstate WATCHDOG_* constants and document why they are necessary.
catch’s picture

Title: Don't duplicate watchdog level constants » Regression: Reinstate WATCHDOG_* constants and document why they are necessary.
Crell’s picture

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

Alan D.’s picture

@catch: lol >> in regards to #55 "A DLL for this PECL extension is currently unavailable."

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ok, 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)?

Garrett Albright’s picture

Status: Needs work » Needs review
FileSize
70.2 KB

Reroll.

keichee’s picture

this makes ubercart and majority of modules to comply with this.. as they are all broken, specially ubercart that uses

Error

The website encountered an unexpected error. Please try again later.
Error message
Notice: Use of undefined constant WATCHDOG_ERROR - assumed 'WATCHDOG_ERROR' in uc_google_checkout_cart_form() (line 471 of C:\proprietary\sites\all\modules\ubercart-7.x-3.x\payment\uc_google_checkout\uc_google_checkout.module).
PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'WATCHDOG_ERROR' for column 'severity' at row 1: INSERT INTO {watchdog} (uid, type, message, variables, severity, link, location, referer, hostname, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => google [:db_insert_placeholder_2] => Google Checkout is enabled, but no Merchant ID found. [:db_insert_placeholder_3] => a:0:{} [:db_insert_placeholder_4] => WATCHDOG_ERROR [:db_insert_placeholder_5] => admin/store/settings/google_checkout [:db_insert_placeholder_6] => http://127.0.0.1:500/admin/store/settings/cart/panes [:db_insert_placeholder_7] => http://127.0.0.1:500/admin/store/settings/cart [:db_insert_placeholder_8] => 127.0.0.1 [:db_insert_placeholder_9] => 1309474801 ) in dblog_watchdog() (line 157 of C:\proprietary\modules\dblog\dblog.module).
pillarsdotnet’s picture

Re-uploading so that testbot can verify that it applies:

  • watchdog_constants-1136130-61-revert.patch
    D8-only patch to change back from LOG_* to WATCHDOG_*.
  • watchdog_constants-1136130-61-followup-1-d8.patch
    D8/D7 patch to add documentation explaining why we can't use the built-in LOG_* constants, as suggested by Crell in #33.
  • watchdog_constants-1136130-61-followup-2-d8.patch
    D8/D7 patch to consolidate the redundant documentation bits into a @docgroup, as suggested by catch in #36.
Crell’s picture

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

keichee’s picture

FileSize
93.11 KB

attached, as per request

catch’s picture

FileSize
69.66 KB

Rolled the 'followup-2' patch into #63 so we just add the defgroup. Hopefully this one's ready.

pillarsdotnet’s picture

You 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".

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

Filed a change request upstream: https://bugs.php.net/bug.php?id=55129

webchick’s picture

Status: Reviewed & tested by the community » Fixed

YAY. Thanks. And thanks sun, for filing the upstream bug.

Committed and pushed to 8.x. Now to go fix a critical bug. :)

pillarsdotnet’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Crell » pillarsdotnet
Status: Fixed » Needs review
Issue tags: -Regression +Documentation, +Needs backport to D7
FileSize
6.49 KB

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

keichee’s picture

wow many thanks for the commit on the git!

catch’s picture

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

catch’s picture

Status: Needs review » Reviewed & tested by the community

Forgot status change.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, cool. This will stop us from making this mistake in the future.

Committed and pushed to 7.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.