Regression: Convert session.inc to the new database API (and _sess_write should use a merge) [#297860]

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

#25	297860-session-inc-dbtng.patch	5.46 KB	Damien Tournoud

#23	297960-session-sdo.patch	5.36 KB	Damien Tournoud

#11	session_revert-297860-10.patch	6.6 KB	pwolanin

#9	session_revert.patch	7.69 KB	catch

#2	session-pdo.patch	5.04 KB	Damien Tournoud

Comment #1

dmitrig01 CreditAttribution: dmitrig01 commented 20 August 2008 at 22:54

Status:

Active

» Postponed

Log in or register to post comments

File	Size
session-pdo.patch	5.04 KB

Comment #3

Damien Tournoud CreditAttribution: Damien Tournoud commented 22 August 2008 at 10:51

Session tests pass seamlessly on MySQL with the patch.

Log in or register to post comments

Comment #4

Dries CreditAttribution: Dries commented 22 August 2008 at 12:49

Status:

Needs review

» Needs work

We can't remove the COOKIE-test for performance reasons. That little check buys us a ton of performance on big sites.

Log in or register to post comments

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 22 August 2008 at 13:01

@Dries: the cookie test is NOT removed.

That's the job of this code block:

  // If saving of session data is disabled or if the client doesn't have a session,
  // and one isn't being created ($value), do nothing. This keeps crawlers out of
  // the session table. This reduces memory and server load, and gives more useful
  // statistics. We can't eliminate anonymous session table rows without breaking
  // the "Who's Online" block.
  if (!session_save_session() || (empty($_COOKIE[session_name()]) && empty($value))) {
    return TRUE;
  }

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

Log in or register to post comments

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 22 August 2008 at 13:03

Note: that last check was already removed in #213699, but Crell reverted it by mistake.

Log in or register to post comments

Comment #7

Damien Tournoud CreditAttribution: Damien Tournoud commented 22 August 2008 at 13:03

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #8

Dries CreditAttribution: Dries commented 23 August 2008 at 07:14

Status:

Needs review

» Fixed

Damien: ah, yes, you're right. Committed to CVS HEAD. Thanks!

Log in or register to post comments

Comment #9

catch

he/him

English

CreditAttribution: catch commented 31 August 2008 at 09:33

Title:	[after DB:TNG] sess_write should use a merge	» Tests broken: sess_write should use a merge
Status:	Fixed	» Needs review

File	Size
session_revert.patch	7.69 KB

This broke Blog API, Path, XML-RPC and Actions configuration tests.

Here's a revert.

http://ca.tchpole.net/node/3 :( :( :(

Log in or register to post comments

Comment #10

catch

he/him

English

CreditAttribution: catch commented 31 August 2008 at 10:24

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

Log in or register to post comments

Comment #11

pwolanin CreditAttribution: pwolanin commented 31 August 2008 at 12:31

File	Size
session_revert-297860-10.patch	6.6 KB

patch 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

Log in or register to post comments

Comment #12

pwolanin CreditAttribution: pwolanin commented 31 August 2008 at 12:46

Status:

Needs review

» Reviewed & tested by the community

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

Log in or register to post comments

Comment #13

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 August 2008 at 12:51

Status:

Reviewed & tested by the community

» Fixed

Ok. Reverted the reversion to the reverted stuff. Hopefully all tests should pass now.

Log in or register to post comments

Comment #14

pwolanin CreditAttribution: pwolanin commented 31 August 2008 at 13:17

Status:

Fixed

» Needs work

really 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().

Log in or register to post comments

Comment #15

catch

he/him

English

CreditAttribution: catch commented 1 September 2008 at 08:53

Title:

Tests broken: sess_write should use a merge

» Regression in sess_write (and it should use a merge)

retitling.

Log in or register to post comments

Comment #16

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 September 2008 at 09:16

Status:

Needs work

» Reviewed & tested by the community

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

Log in or register to post comments

Comment #17

pwolanin CreditAttribution: pwolanin commented 1 September 2008 at 15:08

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

Log in or register to post comments

Comment #18

pwolanin CreditAttribution: pwolanin commented 1 September 2008 at 15:08

Status:

Reviewed & tested by the community

» Needs work

Log in or register to post comments

Comment #19

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 September 2008 at 15:21

The documentation of register_shutdown_function() indicates:

Note: Working directory of the script can change inside the shutdown function under some web servers, e.g. Apache.

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.

Log in or register to post comments

Comment #20

pwolanin CreditAttribution: pwolanin commented 1 September 2008 at 16:28

@Damien - ok, then the patch in #2 should be combined with #259623: Broken autoloader: convert includes/requires to use absolute paths in one patch.

Log in or register to post comments

Comment #21

catch

he/him

English

CreditAttribution: catch commented 1 September 2008 at 17:25

Status:

Needs work

» Postponed

Marking postponed until the autoloader issue is fixed.

Log in or register to post comments

Comment #22

arhak CreditAttribution: arhak commented 18 September 2008 at 20:58

subscribe

Log in or register to post comments

Comment #23

Damien Tournoud CreditAttribution: Damien Tournoud commented 21 September 2008 at 23:57

Status:

Postponed

» Needs review

File	Size
297960-session-sdo.patch	5.36 KB

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

Log in or register to post comments

Comment #24

Crell CreditAttribution: Crell commented 7 October 2008 at 06:50

Status:

Needs review

» Needs work

If 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(). :-(

Log in or register to post comments

Comment #25

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 18:35

Title:	Regression in sess_write (and it should use a merge)	» Convert session.inc to the new database API (and _sess_write should use a merge)
Assigned:	Unassigned	» Damien Tournoud
Category:	bug	» task
Status:	Needs work	» Needs review

File	Size
297860-session-inc-dbtng.patch	5.46 KB

Here is a full convert of session.inc to the new database API.

Log in or register to post comments

Comment #26

Crell CreditAttribution: Crell commented 1 November 2008 at 19:53

Status:

Needs review

» Needs work

Line 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!

Log in or register to post comments

Comment #27

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 20:00

File	Size
297860-session-inc-dbtng.patch	5.69 KB

Log in or register to post comments

Comment #28

Dries CreditAttribution: Dries commented 1 November 2008 at 20:00

Not quite OK yet: time() should be REQUEST_TIME.

Log in or register to post comments

Comment #29

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 20:00

Status:

Needs work

» Needs review

Cross posted with Dries.

Log in or register to post comments

Comment #30

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 20:07

File	Size
297860-session-inc-dbtng.patch	5.71 KB

Fixed two other time() calls.

Log in or register to post comments

Comment #31

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 20:12

File	Size
297860-session-inc-dbtng.patch	5.71 KB

Fixed more code style issues.

Log in or register to post comments

Comment #32

Damien Tournoud CreditAttribution: Damien Tournoud commented 1 November 2008 at 20:14

File	Size
297860-session-inc-dbtng.patch	5.75 KB

And a not messed-up _sess_destroy_sid().

Log in or register to post comments

Comment #33

Crell CreditAttribution: Crell commented 1 November 2008 at 20:21

Status:

Needs review

» Reviewed & tested by the community

#32 applies, session tests all pass, and I can't find anything else to complain about with the coding style. Thanks, Damien!

Log in or register to post comments

Comment #34

Dries CreditAttribution: Dries commented 1 November 2008 at 21:27

Status:

Reviewed & tested by the community

» Fixed

Excellent. Committed to CVS HEAD. Thanks!

Log in or register to post comments

Comment #35

Anonymous (not verified) CreditAttribution: Anonymous commented 15 November 2008 at 21:46

Status:

Fixed

» Closed (fixed)

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

Log in or register to post comments

Comment #36

sun

German

Karlsruhe

CreditAttribution: sun commented 18 December 2009 at 01:18

Title:	Convert session.inc to the new database API (and _sess_write should use a merge)	» Regression: Convert session.inc to the new database API (and _sess_write should use a merge)
Category:	task	» bug
Status:	Closed (fixed)	» Active

File	Size
drupal.xmlrpc-clear-errors.36.patch	1.26 KB

Sorry, folks. CVS annotate revealed this old issue.

+++ includes/xmlrpc.inc	31 Aug 2008 09:30:07 -0000
@@ -342,7 +342,7 @@ EOD;
-function xmlrpc_error($code = NULL, $message = NULL, $reset = FALSE) {
+function xmlrpc_error($code = NULL, $message = NULL) {

@@ -351,9 +351,6 @@ function xmlrpc_error($code = NULL, $mes
-  elseif ($reset) {
-    $xmlrpc_error = NULL;
-  }

@@ -430,7 +427,6 @@ function xmlrpc_base64_get_xml($xmlrpc_b
 function _xmlrpc() {
...
-  xmlrpc_clear_error();

@@ -479,10 +475,3 @@ function xmlrpc_error_msg() {
-/**
- * Clears any previous error.
- */
-function xmlrpc_clear_error() {
-  xmlrpc_error(NULL, NULL, TRUE);
-}

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.

Log in or register to post comments

Comment #37

sun

German

Karlsruhe

CreditAttribution: sun commented 18 December 2009 at 01:19

Status:

Active

» Needs review

Log in or register to post comments

Comment #38

moshe weitzman CreditAttribution: moshe weitzman commented 18 December 2009 at 03:26

i don't see that xmlrpc code in the patches here.

Log in or register to post comments

Comment #39

sun

German

Karlsruhe

CreditAttribution: sun commented 18 December 2009 at 14:12

@see #11, the first patch that was committed

Log in or register to post comments

Comment #40

Dries CreditAttribution: Dries commented 18 December 2009 at 16:30

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