in clipper module I ran against a breally odd issue. When enabled, about half the other modules' nodeapi hooks would not get called.
flevour hopped on for a wild bughunt, and came with this solution:

instead of doing a foreach (module_implements('nodeapi') as $name) { we call the module_implements('nodeapi') before.

Here is how to reproduce:
Just add a random node_load(1) to any nodeapi insert, update or validate, in a random module (well, not upload, because that is the last one ran, most prolly :) ).
You wil then notice that half of the data in your node/add or /edit screens are no longer saved, nor checked.

This tiny patch fixes the issue. I have no clue why, but it does fix it. :)

CommentFileSizeAuthor
#8 php_bug_1.patch570 byteschx
#6 php_bug_0.patch521 byteschx
#5 php_bug.patch312 byteschx
#3 node_module_nodeapi_oddness_0.patch900 bytesBèr Kessels
node_module_nodeapi_oddness.patch873 bytesBèr Kessels

Comments

chx’s picture

chx commented

This is sheer impossible. I'll investigate more but there is no way module_implements' internal cache (a static variable) gets overridden, after that cache is build it's only returned. There is no reset facility as there is no need.

chx’s picture

chx commented

I am babbling because of the shock.

foreach operates on a copy. it does not matter whether module_implements is immutable. Ber, what PHP version do you run?

Bèr Kessels’s picture

Bèr Kessels commented
StatusFileSize
new node_module_nodeapi_oddness_0.patch900 bytes

I find it funny that you say it is impossible, while the preactice proves that this fixes the issue.

"One should really learn to beleive reality, even if theory doesnt allow that reality"

anyhow, a slighly modded patch, one that adds a comment.

chx’s picture

chx commented

As said, I am babbling.

Second, while the patch is patch good, I told you, I fear about what else is broken. Not what your patch brokes, but there is an unknown cause and that cause might have broken other stuff too.

chx’s picture

chx commented
Assigned: Unassigned » chx
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new php_bug.patch312 bytes

I chatted with Goba and we worked around the problem at its root.

chx’s picture

chx commented
StatusFileSize
new php_bug_0.patch521 bytes

Much better comments. This is super-duper hard.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

This fixes the issue much more generally, and should be committed. (The issue is that arrays in arrays are stored with references).

+1

chx’s picture

chx commented
StatusFileSize
new php_bug_1.patch570 bytes

Linking relevant Zend docs.

dries’s picture

dries commented

I'm not sure I understand the PHP comment. What do you mean with 'the array pointer will be off'? Can you try to clarify that part of the code comment? Thanks.

Bèr Kessels’s picture

Bèr Kessels commented

@dries. I have no clue, really :p This array-memoy-space-pointer-stuff is way over my head. CHX, mind sharing your knowledge here?

Oh, and chx, do you mind changing this to 'normal' instead of critical? so far, in about two years, i'm the only one who ever encountered it, and that even for a rather unpopular module.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

I fixed up the wording/explanation a bit and committed this patch. Thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)