Fix install/update issues.

My bad - did not know that that was invalid syntax. Attached patch should remove that space.

I wonder if I have done similar in other (non privatemsg) places too?

committed.

This is a subpart of my patch at #410374: Upgrade problems, which wasn't much welcome in it's current form, and I'm not going to push on that one, as fixing upgrade path doesn't make much sense IMO (until privatemsg is feature-complete at the very least, let alone reaching a *real* Release Candidate state).

But still, let me point out, that the very bit of code, as seen in this patch here, have anoter problem: db_fetch_array/db_fetch_object mismatch. Either re-open this, or chip in to #410374: Upgrade problems - I let this decision to others.

Attached patch:

1. fixes the db_fetch_array/db_fetch_object mismatches.
2. Moves the update 6001 function to after 6000 (and before 6002).

None of these cause functional changes, but should make things more consistent (especially when thinking on where to place update 6003).

This looks good on brief read, but I didn't have a chance to actually run the code yet, so not setting RTBC although I feel like it. (Don't rely on me as tester, as my time is very limited, and I *really* need to hack the update 6000 badly to make it finish on my site, per first point below.) BTW the type mismatch surely *is* a functional change - without that fix, all my tags import as empty named.

-----------------

Summarizing, what problems I had over at #410374: Upgrade problems (which may be closed as duplicate, if these points are going to be resolved here):

- The existing single update function is a monster. Needs to be split, to finish without hitting PHP time limit.
This bit most probably needs to turn update 6000 to a multi-pass, with the use of sandbox (as seen in some core updates, for example). It times out on my site for sure. And since there are *several* time-consuming tasks in there, only just splitting along the commented "steps" is going to improve the scalability multiple times.

- The attempt on set_time_limit() to unlimited won't work on many (most?) real sites, hosting accounts mostly run in safe mode.
Now I suggest to just try @set_time_limit() to avoid warnings. This doesn't make the above point go away though, as most hosting providers don't allow the time limit to be changed.

- The code enables two modules dependent on privatemsg, without ensuring that privatemsg is enabled (6.x core run updates of disabled modules too!). Quite a few upgrade tutorials tell to disable contributed modules, then enable one-by-one. This way, we get into a state, where privatemsg is disabled, but still the checkbox on modules page is grayed-out due to dependency of (force-enabled) sub-modules.
Still valid. Upgrading along common suggestions leads to privatemsg disabled and impossible to enable.

- The code around transforming old folders into tags throws warnings, and imports all tags as empty-named. There are two problems: db_fetch_array / db_fetch_object mismatch, and extra space (syntax error) after COUNT in the query.
Fixed by the two patches here. Thanks!

- Final cleanup attempts to drop 'privatemsg_block_user' table, even if that does not exist.
Still valid. I'm upgrading a bit older 5.x site, and it doesn't have that table. Update throws warning.

Plus, no old messages appear to be tagged according to their old folder-location?
I didn't notice before, but this is most probably caused by the second type mismatch part, and so fixed by the above patch.

But the tags part probably needs more changes (I believe this is being discussed somewhere else?), because the old folders were private (per user), and now a mix of all these is exposed to all users as tags.
This is still valid, but it's probably out of scope here. Either the tags should become per-user, or the old folders should *not* be converted to tags. If I was a regular user, and had something *very* private as my personal mail sub-folder name, I certainly won't like it to transform into public, admin-defined tag, that I can't delete. But this happens now.

On the first two points - yes I agree that the update function is a monster and set_time_limit() is a hack to force it to work.

However, splitting the function up is no better as individual parts can also go over the time limit. The proper solution as you mentioned is the session/sandbox/batch api. I however do not know how to do that and when I tried I failed miserably, so we are at the place where we have the current code - which has worked for people including me.

Patch now also checks if the privatemsg_block_user table exists before deleting.

the tagging issue is not being addressed by this patch though and the way the tagging is going atleast in the short term is to make tags "appear" to be per user instead of actually being per user. This allows for more flexibility.

However, splitting the function up is no better as individual parts can also go over the time limit.

Sure, but the probability of a fail is much lower than now, so it's an improvement already. Anyway, as pointed out on the other issue, the splitting needs batch api/sandbox already, now that there are more update functions after.

Briefly looking at the patch:

if (!module_exists('privatemsg')) {
    module_exists('privatemsg')
  }

This doesn't look right to me - did you mean module_enable()? (Or we may enable by direct query to {system} table, too.)

I think we should put that @set_time_limit() in, to suppress warnings at least - it might work on small databases even if the limit setting failed.

Going to bed now, goodnight.

oops - fixed.

Not a fan of suppressing the error messages as if it fails, people will not know why.

Needs a re-roll as the 6001 update function has already been moved.

rerolled.

Patch still applies and looks like an important bugfix. Can someone please confirm that this does still work as I don't have an Drupal5 dev environment at hand?

This has been fixed in 6.x-1.x-dev.