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.
It currently doesn't... which means that, for example, if you call drupal_get_bootstrap_phase()
you will get DRUPAL_BOOTSTRAP_FULL
.
Is there any reason not to do that?
Comment | File | Size | Author |
---|---|---|---|
#38 | simpltest_fix_verbose.patch | 949 bytes | yched |
#37 | simpltest_fix_verbose.patch | 942 bytes | yched |
#34 | 601584-destory-statics-34.patch | 5.24 KB | yched |
#28 | 601584-destory-statics-27.patch | 7.88 KB | effulgentsia |
#27 | 601584-destory-statics-27.patch | 7.88 KB | effulgentsia |
Comments
Comment #1
boombatower CreditAttribution: boombatower commentedDon't know. Have been wondering the same.
Here's a patch..lets see how bad it is.
Comment #2
boombatower CreditAttribution: boombatower commentedWoops other patch included.
Comment #3
boombatower CreditAttribution: boombatower commentedumm...359 :)
Comment #4
boombatower CreditAttribution: boombatower commentedOk so if I try to run the tests not in the list I get an awesome error with no stack trace...
Here is what's in there when it tries to fry it. Assuming one of the systems can't recover.
Comment #5
boombatower CreditAttribution: boombatower commentedEventually got
after moving it.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commenteddebug code in there ... i do think that resetting statics makes sense. thats why we built this whole static feature.
Comment #7
boombatower CreditAttribution: boombatower commentedRemoved debug, still won't run all tests most likely.
Comment #8
catchTry with #572452: drupal_get_filename() and drupal_load() should not use drupal_static() ?
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedThe problem with why so few tests ran in prior patches is that Batch API doesn't survive a reset, and drupal_static_reset() is broken anyway (#620688: drupal_static_reset is broken). This patch merges the code from http://drupal.org/node/620688#comment-2250000. Let's see what bot thinks.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedThis solves the 3 exceptions from #9's test run. There remains the 1 failure in translatable fields. I need to put this down for now, but if someone can debug what's going on with translatable fields, and start thinking about how we need to refactor drupal_bootstrap() and Batch API, so that their variables can survive reset, then we should be golden.
Comment #13
yched CreditAttribution: yched commentedHm, batch API is sufficiently low level (required by install, update, testing...), that I don't think batch_get() should use drupal_static() to begin with. It's precisely not something that we want to be resettable from the outside...
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThanks, yched, excellent point. Besides, it returns a reference, so anything wanting to mess with it can call batch_get() and manipulate the result. Also, for reasons commented in the patch, the drupal_bootstrap() statics shouldn't use drupal_static() either. This patch is therefore clean, except for the Field Translation failure. CNR so bot can re-confirm that that's the only remaining failure.
Comment #16
yched CreditAttribution: yched commentedI don't think I get this. Before #422362: convert form.inc to use new static caching API, the function was :
Worked fine - probably thanks to the & in front of the function name ? (just like drupal_static(), BTW)
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedYou're right. Statics can't be set to references, and I assumed that meant they can't be returned by reference, but I just tested it, and it turns out that returning static by reference works fine, so I'll include the cleanup in the next patch, once I or someone figures out what's causing the Field Translation test failure.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedFound it! This one should pass.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedRe-rolled due to #620688: drupal_static_reset is broken. Any takers on RTBC'ing it or providing feedback?
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedBetter re-roll.
Comment #21
yched CreditAttribution: yched commentedNitpick in batch_get():
"The only legitimate API for resetting batch information is batch_set()."
That's inaccurate. batch_set() only lets you add new sets to the batch. The only way to reset the batch to array() is:
Frankly, I'm not sure it's worth mentioning, I don't see the use case.
Comment #22
effulgentsia CreditAttribution: effulgentsia commented@yched: better?
Comment #23
effulgentsia CreditAttribution: effulgentsia commented@yched: I just re-read batch_set() and now see what you meant. So, how's this comment?
Comment #24
yched CreditAttribution: yched commented@effulgentsia: well, IMO it would be OK to stop at "we specifically do not want a global drupal_static_reset() resetting the batch information", but the text is correct ;-). Thanks.
Comment #25
effulgentsia CreditAttribution: effulgentsia commented@yched: Thanks. I'm okay with stopping where you suggest if you or someone else prefers that. My motivation in having the more verbose text is to stop a core developer in the future from changing this back to drupal_static() because they have a use-case for needing to reset.
Comment #26
chx CreditAttribution: chx commentedfinal_phase says
+ // Not drupal_static(), because the only legitimate API to control this is to
+ // call drupal_static() with a new phase parameter.
heh no. call drupal_bootstrap with a new phase parameter.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedD'oh! Thanks for catching that!
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedI guess bot was down when #27 was posted, and even though it passed (click "view details"), it's not the nice and shiny green we all like to see, so re-uploading to get the green.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedThis still needs someone to set it to RTBC.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedI'm gonna risk breaking protocol here, and will set my own patch to RTBC. Because:
1) The concept of resetting all statics during the setup of a test is supported by many people, on this issue and others. See #6.
2) This specific implementation has been reviewed by chx and yched, and both of them reported only minor issues with comments, which have been addressed.
3) It just makes sense to get this into HEAD sooner rather than later, since it may help uncover bugs with other patches in the issue queue.
Comment #31
Dries CreditAttribution: Dries commentedI think this is a smart thing to do because it will make things more robust. However, the patch needs a reroll because of the field.test changes.
Comment #32
Dries CreditAttribution: Dries commentedComment #33
yched CreditAttribution: yched commentedSome of this got committed along with #634440: Remove auto-rebuilding magic for $form_state['storage']
Comment #34
yched CreditAttribution: yched commentedHere's a reroll for the rest of the patch, and for the field.test hunk after #638356: Reorganize field_test.module
Comment #35
Dries CreditAttribution: Dries commentedAlright-y, committed to CVS HEAD. Hopefully the tests will all pass.
Comment #36
yched CreditAttribution: yched commentedHm, is it possible that this patch broke the 'verbose' information on test results ?
Even with 'verbose' checkbox set on admin/config/development/testing/settings, test results do not contain the links to the HTML pages after a drupalPost / drupalGet. Worked yesterday :-(.
Comment #37
yched CreditAttribution: yched commentedYup, simpletest_verbose() had a drupal_static()-based $verbose variable. That one should be a regular static as well.
Comment #38
yched CreditAttribution: yched commentedEr, let's explicitly initialize it to NULL like the other statics in the function.
Comment #39
Dries CreditAttribution: Dries commentedCommitted. Thanks yched.