Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The DB:TNG patch reverted http://drupal.org/node/213699. sess_write should use a merge anyway.
Comment | File | Size | Author |
---|---|---|---|
#36 | drupal.xmlrpc-clear-errors.36.patch | 1.26 KB | sun |
#32 | 297860-session-inc-dbtng.patch | 5.75 KB | Damien Tournoud |
#31 | 297860-session-inc-dbtng.patch | 5.71 KB | Damien Tournoud |
#30 | 297860-session-inc-dbtng.patch | 5.71 KB | Damien Tournoud |
#27 | 297860-session-inc-dbtng.patch | 5.69 KB | Damien Tournoud |
Comments
Comment #1
dmitrig01 CreditAttribution: dmitrig01 commentedComment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedSession tests pass seamlessly on MySQL with the patch.
Comment #4
Dries CreditAttribution: Dries commentedWe can't remove the COOKIE-test for performance reasons. That little check buys us a ton of performance on big sites.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commented@Dries: the cookie test is NOT removed.
That's the job of this code block:
The second check is an historical remain of a previous refactoring of that function, and always match, as I explained in detail on http://drupal.org/node/213699#comment-865231
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedNote: that last check was already removed in #213699, but Crell reverted it by mistake.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #8
Dries CreditAttribution: Dries commentedDamien: ah, yes, you're right. Committed to CVS HEAD. Thanks!
Comment #9
catchThis broke Blog API, Path, XML-RPC and Actions configuration tests.
Here's a revert.
http://ca.tchpole.net/node/3 :( :( :(
Comment #10
catchWithout debug code. Trying to figure this out with cwgordon yesterday we got as far as simpletest finding drupal_set_messages which had already appeared being displayed again after a drupalGet.
Comment #11
pwolanin CreditAttribution: pwolanin commentedpatch was missing here it is.
Running all tests, some exceptions:
- Query altering tests, part 2: 20 passes, 0 fails, 1 exception
- DBLog functionality: 309 passes, 0 fails, 2 exceptions
Fails on upload - maybe my setup?
Upload functionality: 124 passes, 3 fails, 0 exceptions
Comment #12
pwolanin CreditAttribution: pwolanin commentedThe upload tests fail for me with or without the patch (but pass for webchick). I think a rollback (i.e. commit the patch above) of this until it can be fixed to not break tests is the right way forward for the moment.
Comment #13
webchickOk. Reverted the reversion to the reverted stuff. Hopefully all tests should pass now.
Comment #14
pwolanin CreditAttribution: pwolanin commentedreally back to CNW - the patch is part of the follow-on DB conversion, but needs to be re-worked to find why tests broke.
Also, I now get upload passing - I needed to delete files/simpletest when re-installing HEAD since some test files were missing and not created due to the logic in simpletest_install().
Comment #15
catchretitling.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry guys, but you lost your time on this one.
The issue is known (from me, at least): the current path can change when PHP enters shutdown functions. So we must use absolute paths everywhere in the autoloader. I've been desperately trying to get some attention on #259623: Broken autoloader: convert includes/requires to use absolute paths for the past few days, but without luck.
Please reapply the patch from #2 and review #259623. That patch is not broken. Our autoloader is (well, technically PHP is broken, but that's another story).
Comment #17
pwolanin CreditAttribution: pwolanin commented@Damien - please clarify. The commit of #2 was reverted by the patch above, so #2 needs to be revised such that it doesn't cause random test breakage (or combined with another issue).
Comment #18
pwolanin CreditAttribution: pwolanin commentedComment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe documentation of register_shutdown_function() indicates:
When sess_write is called (via session_write_close() registered as a shutdown function in sess_read() in order to guarantee that it is called before objects destructors are fired), the working directory may thus have been changed.
When db_merge() is called, the database layer need to load MergeQuery* objects. If those were not used during the page processing, the autoloader tries to load them, but fails because the working directory is incorrect.
The patch in #2 is correct. What cause the breakage is the failure of our autoloader. We should thus fix #259623: Broken autoloader: convert includes/requires to use absolute paths.
Comment #20
pwolanin CreditAttribution: pwolanin commented@Damien - ok, then the patch in #2 should be combined with #259623: Broken autoloader: convert includes/requires to use absolute paths in one patch.
Comment #21
catchMarking postponed until the autoloader issue is fixed.
Comment #22
arhak CreditAttribution: arhak commentedsubscribe
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedNow that #259623: Broken autoloader: convert includes/requires to use absolute paths is in, here is a reroll of the patch in #2. I told you that this patch was not to blame.
Rerun the whole test suite... 100% pass.
Comment #24
Crell CreditAttribution: Crell commentedIf we're going to update all the queries, let's just do a full DBTNG conversion on session.inc and be done with it. :-)
Delete queries should use the new query builder. db_fetch_*() is also now deprecated in favor of foreach(). db_result() is also deprecated in favor of fetchField().
I think we seem to be establishing a convention of using fetchObject() explicitly rather than just fetch() when directly fetching a record, as that is more self-documenting. Chained query builder calls should also be broken across multiple lines if there are 2 or more chained methods. It looks like a REQUEST_TIME is also being reverted to time(). :-(
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a full convert of session.inc to the new database API.
Comment #26
Crell CreditAttribution: Crell commentedLine 86, use fetchObject() rather than fetch() as it is more self-documenting.
Line 95, that's the sort of slick stuff I was hoping to enable with DBTNG. :-)
Line 141, use REQUEST_TIME, not time(), as it's a tiny bit faster. Same on line 149.
Lines 144, 149, 161, those built queries should be broken out across multiple lines.
Line 254, I don't understand what the () are for. They can be removed.
Should be easy to fix up. Let's get this in!
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #28
Dries CreditAttribution: Dries commentedNot quite OK yet: time() should be REQUEST_TIME.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedCross posted with Dries.
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedFixed two other time() calls.
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedFixed more code style issues.
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd a not messed-up _sess_destroy_sid().
Comment #33
Crell CreditAttribution: Crell commented#32 applies, session tests all pass, and I can't find anything else to complain about with the coding style. Thanks, Damien!
Comment #34
Dries CreditAttribution: Dries commentedExcellent. Committed to CVS HEAD. Thanks!
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #36
sunSorry, folks. CVS annotate revealed this old issue.
These changes should not have been in this patch. They revert the fix from #208270: XML-RPC error handling broken. This is a regression to D6.
Comment #37
sunComment #38
moshe weitzman CreditAttribution: moshe weitzman commentedi don't see that xmlrpc code in the patches here.
Comment #39
sun@see #11, the first patch that was committed
Comment #40
Dries CreditAttribution: Dries commentedThe change in #36 was incorrectly rolled back, it seems. That change was required to make the XML-RPC error handling work. It looks like that patch was accidentally reverted. Because it is accidentally reverted, XML-RPC errors are never reset, which makes it pretty unreasonable to do more than one XML-RPC request per HTTP request. Clearly, we need to fix that. See also #578470: XML-RPC error handling sometimes fails silently.
Comment #41
sun#36 was committed, it seems.