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.
Please ignore this thread. I just need to give testbot a hard time without cluttering up another issue with spam.
Comment | File | Size | Author |
---|---|---|---|
#112 | 1463656-drupal-kernel.patch | 101.64 KB | Crell |
#109 | 1486960_109_drupal-kernel.patch | 100.64 KB | cosmicdreams |
#108 | 1463656-drupal-kernel.patch | 101.54 KB | Crell |
#105 | 1463656-drupal-kernel.patch | 104.55 KB | Crell |
#104 | kernel-benchmark-head.patch | 284 bytes | effulgentsia |
Comments
Comment #2
Crell CreditAttribution: Crell commentedComment #4
Crell CreditAttribution: Crell commentedComment #5
Crell CreditAttribution: Crell commentedTrying again...
Comment #7
deviantintegral CreditAttribution: deviantintegral commentedPulling over the patch from https://drupal.org/node/1486960#comment-5772004
Comment #8
deviantintegral CreditAttribution: deviantintegral commentedComment #9
Crell CreditAttribution: Crell commentedComment #11
Crell CreditAttribution: Crell commentedComment #13
Crell CreditAttribution: Crell commentedComment #15
Crell CreditAttribution: Crell commentedComment #17
Crell CreditAttribution: Crell commentedComment #19
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTesting the over-all effects of the changes.
Comment #21
Crell CreditAttribution: Crell commentedComment #22
Crell CreditAttribution: Crell commentedComment #24
Crell CreditAttribution: Crell commentedComment #25
Crell CreditAttribution: Crell commentedMaybe it jammed? :-(
Comment #27
Crell CreditAttribution: Crell commentedComment #29
Crell CreditAttribution: Crell commentedComment #31
Crell CreditAttribution: Crell commentedThat was totally wrong.
Comment #33
Crell CreditAttribution: Crell commentedComment #35
Crell CreditAttribution: Crell commentedTagging
Comment #36
cosmicdreams CreditAttribution: cosmicdreams commentedComment #38
cosmicdreams CreditAttribution: cosmicdreams commentedOK, I see what I'm doing wrong, I'm not diffing this code with Drupal 8 HEAD. This patch is my first attempt to do this kind of a patch.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedThat's what I get for trying to use format-patch. Here's a regular git diff
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedThanks pifr for finding that. sorry I missed that. Hopefully this one passes.
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedthis one should be smaller and effect fewer files. It's curious to see $_GET['q'] be replaced by current_path() since current_path() returns $_GET['q']. Is this a mechanism to replace how the current path is retrieved in the future? should I spread the change of $_GET['q'] to current_path() to every part of drupal?
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commentedforgot patch
Comment #46
cosmicdreams CreditAttribution: cosmicdreams commentedtesting a hypothesis, the only difference between this patch and 45 is that I've change 12 instances of $_GET['q'] in menu.inc and common.inc to current_path().
Comment #48
Crell CreditAttribution: Crell commentedComment #51
Crell CreditAttribution: Crell commentedComment #53
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTesting how some changes perform.
Comment #55
Crell CreditAttribution: Crell commentedStarting to get up there in size, but should be much better on the test front...
Comment #57
jthorson CreditAttribution: jthorson commentedOnce again, a testbot breaker ... or at least, a testbot plugger ...
Hopefully these can shed some light on things ... This is just a select excerpt; the first one is the most common by about 5 times.
Comment #58
sunAs long as testbots aren't shielded against this attack ;)
the best way to avoid taking them down is to run at least one of the failing test cases locally.
---
That said, since the PSR-0/classloader changes in D8, my local PHP 5.3.6 frequently manages to freeze and take down my local Apache server on fatal or more severe exceptions. Perhaps something similar is happening on testbots? At least I've the impression something odd is going on in our codebase.
Comment #59
Crell CreditAttribution: Crell commentedThis is freshly merged from upstream to include the /index.php/ patch. Let's see what happens...
Comment #61
cosmicdreams CreditAttribution: cosmicdreams commentedThis appears to be the showstopper:
Comment #62
Crell CreditAttribution: Crell commentedComment #64
cosmicdreams CreditAttribution: cosmicdreams commentedSearched through the code and found 5 or 6 instances where the frontpage settings were handled by variable_get, variable_set.
Fixed those and added some more debugging information. (which I will take out if this goes green)
Comment #65
cosmicdreams CreditAttribution: cosmicdreams commentedwoops wrong issue, sorry
Comment #67
Crell CreditAttribution: Crell commentedComment #69
cosmicdreams CreditAttribution: cosmicdreams commented#67: 1463656-drupal-kernel.patch queued for re-testing.
Comment #71
neclimdulComment #72
aspilicious CreditAttribution: aspilicious commentedComment #74
effulgentsia CreditAttribution: effulgentsia commentedThis is #71 plus language negotiation fixes. Depending on test results, I'll commit the interdiff to a suitable branch.
Comment #75
cosmicdreams CreditAttribution: cosmicdreams commented#74: 1486960-kernel-testbot-74.patch queued for re-testing.
Comment #76
effulgentsia CreditAttribution: effulgentsia commented#74 changes some bootstrap logic that affects all non-index.php scripts, like the bot runner, which is why it ran 0 tests. This adds a temporary _drupal_bootstrap_legacy() function to work around that.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedSweet. That's a 90% failure reduction and 70% exception reduction. This should be the same from a test standpoint, but with a cleaner implementation.
Comment #80
cosmicdreams CreditAttribution: cosmicdreams commentedtiny change, a typo may have made one of the tests to fail.
Comment #81
cosmicdreams CreditAttribution: cosmicdreams commentedComment #83
Crell CreditAttribution: Crell commentedComment #85
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #86
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedUgg ... that should have beenWrong, too. Merge it and then diff next time....
, not..
-- poor testbot :/Comment #88
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #90
Crell CreditAttribution: Crell commentedComment #92
Crell CreditAttribution: Crell commentedComment #93
aspilicious CreditAttribution: aspilicious commentedYEAY!!! GREEN!!!
But I noticed something...
Tests are slowed down by 10! minutes with this patch.
Seriously? Why?
Comment #94
pounardCongrats!
Comment #95
cosmicdreams CreditAttribution: cosmicdreams commentedOMG Green!
Reviewing...
Comment #96
andypostIs this a hack? Suppose better to add another array argument to url()
Comment #97
Crell CreditAttribution: Crell commentedPlease don't post code reviews here. Let's keep those in the main thread. This issue is just for playing testbot pingpong. :-)
Comment #98
effulgentsia CreditAttribution: effulgentsia commentedThis is same as #1463656-118: Add a Drupal kernel; leverage HttpFoundation and HttpKernel but without the .htaccess change. I'm curious if that change is required for tests to pass.
Comment #99
Crell CreditAttribution: Crell commentedeffulgentsia: Please remember to use the sandbox, since that's what we're merging from. Straight up patches are just duplicated work. :-(
Comment #100
Crell CreditAttribution: Crell commentedComment #101
effulgentsia CreditAttribution: effulgentsia commentedSame patch as in #1463656-125: Add a Drupal kernel; leverage HttpFoundation and HttpKernel, minus xmlrpc.test changes. If this passes, I will open a separate core issue for that, and a wscci sandbox issue to remove it from the kernel work.
Comment #102
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@effulgentsia: Mhh ... looks like this has been solved (somehow) in the mean-time. Ensured it passes with clean URLs as well. I looked at the history and made sure it indeed failed at some point. Didn't bisect it, yet. But as long as it passes that's awesome. Do we even need a follow-up?
Comment #103
effulgentsia CreditAttribution: effulgentsia commentedYes. See #1593674: Remove unrelated change to xmlrpc.test for details.
Comment #104
effulgentsia CreditAttribution: effulgentsia commentedTiming testbot as part of #1578090: Benchmark/profile kernel. Will post analysis there.
Comment #105
Crell CreditAttribution: Crell commentedComment #106
aspilicious CreditAttribution: aspilicious commented#105: 1463656-drupal-kernel.patch queued for re-testing.
Comment #108
Crell CreditAttribution: Crell commentedHopefully the last one...
Comment #109
cosmicdreams CreditAttribution: cosmicdreams commentedAn attempt to reroll after all the PSR-0 tests were committed
Comment #110
aspilicious CreditAttribution: aspilicious commentedYou're missing the node test stuff
Comment #112
Crell CreditAttribution: Crell commentedHopefully for the last time...
Comment #113
aspilicious CreditAttribution: aspilicious commentedA random doc review so I remember all these issues I found for followups.
I would group symfony stuff but thats just me
Do we need the first backslasg?
probabaly "@return string"
I still don't like this...
Has some undocumented functions, but is probably going to be rewritten...
This controller has a lot of functions with missing @return documentation.
Request should be removed
We should rewrite this in drupal api docs
Bigger than 80 chars?
26 days to next Drupal core point release.
Comment #114
pounardAgree.
Comment #115
Crell CreditAttribution: Crell commentedThe kernel patch is now in(!), so we can close this issue, too.
aspilicious, can you open a new kernel-followup tagged issue with those tweaks?
GO TEAM!