Now that #652566: Support multiple download files per release in release history XML is committed and deployed on updates.drupal.org, the release history XML feed generates PHP warnings on the legacy crappy PHP4 XML parser in update.module in D6 core. We knew this was going to happen, but figured it was a lesser evil to the complication of a whole separate tree of release history XML files, especially given that official releases of D6 core don't ship with E_ALL enabled.
Luckily, it's a trivial patch that's already been well tested by both myself and mikey_p. Instead of it being lost and forgetten over at #555362-1: Support multiple download links in the available updates report (.tar.gz + .zip packages like on drupal.org) (which is probably never going to be backported to D6, anyway), I wanted to open a new issue for the patch:
http://drupal.org/files/issues/555362-1.update_parse_files.D6.patch
Over a year later, it still applies cleanly to update.fetch.inc. ;)
Comment | File | Size | Author |
---|---|---|---|
#42 | 555362-1.update_parse_files.D6.patch | 1.75 KB | westwesterson |
Comments
Comment #1
dwwJust to be clear: D7 core doesn't need this fix, since the legacy PHP4 parser is already gone. There's no "Fix in HEAD, then backport" here because this bug only effects D6 core...
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedLooks fine to me, inasmuch as I remember that crappy XML parser. If dww and mikey_p have both tested it, then +1.
Comment #3
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedWorked for me
Comment #4
westwesterson CreditAttribution: westwesterson commentedtested! Works!
Comment #5
greg.harvey+1 - applied cleanly and works in Pressflow too, FYI...
Comment #6
justin.cherniak CreditAttribution: justin.cherniak commentedSo if this works for everyone, what is the possibility of getting it in the next Drupal release?
Comment #7
dwwHopefully there's a strong possibility of getting this committed before the next release. To help, the folks who test this should document their environment when they reply to say "it works", at least the php version. We need to have this manually confirmed on php 4 vs. 5, possibly multiple versions of each (e.g. 4.3, 4.4, 5.2 and 5.3).
Thanks,
-Derek
Comment #8
justin.cherniak CreditAttribution: justin.cherniak commentedWorks for me using php 5.3
Comment #9
joshuajabbour CreditAttribution: joshuajabbour commentedworks on php 5.2.13
Comment #10
greg.harvey@dww - sorry, update based on #7: PHP 5.3 + latest Drupal and Pressflow cores - it worked fine.
Comment #11
dwwHas anyone tested this with PHP 4.4 (and perhaps 4.3) yet? Once you have, please set back to RTBC. Thanks!
Comment #12
alberto56 CreditAttribution: alberto56 commentedsubscribing, thanks.
Comment #13
Shadlington CreditAttribution: Shadlington commentedSub'd
Comment #14
caschbre CreditAttribution: caschbre commentedI applied the patch and it appears to have resolved the issue for me as well.
Comment #15
vectoroc CreditAttribution: vectoroc commentedsubscribing
Comment #16
dww@caschbre: What version of PHP are you using?
@all: We need testers for this patch with PHP4.
Thanks,
-Derek
Comment #17
caschbre CreditAttribution: caschbre commentedI'm using PHP 5.2.14
Comment #18
klonos@dww: I know we need people in php4 to test this and see if anything breaks Derek, but the fact remains that by waiting for this to happen all of us in php5 need to address *actual* issues we are faced with while we wait for people to test for any *possible* issues. Besides, servers still in php4 possibly won't even have updated installations of D6 to begin with -not 6.x-dev-. So we might wait for ever here and have (yet) another patch to remember to maintain in our setups during each update.
Please push this and let people still in php4 file separate issues if they (ever) have problems.
Thanx for considering this.
Comment #19
dwwI can't push this, only Gabor. I'm just trying to get people to focus on steps that will aid that happening.
Comment #20
klonosYeah, the "Please push this..." paragraph was not for you alone/specifically. It's just that you are the one that created this patch and even though you cannot actually push (as in committing it yourself) you can "push" others to push it. Is Gábor the only one that can give this a proper review? Can't we "ping" anyone else?
Thanx for your work btw ;)
Comment #21
dwwRight, and I'm trying to say, if we set this to RTBC (which is when Gabor is likely to look at it), if there's no comment that clearly confirms it was tested in PHP4, he's just going to set it back to 'needs review', not 'fixed'...
Comment #22
klonosPoint taken.
Comment #23
caschbre CreditAttribution: caschbre commentedSo what needs to be done to test in PHP4? I typically use the acquia DAMP stack so I don't mess with PHP, apache, etc.
Would I need to create a virtual machine then manually load php4, mysql, and apache?
Comment #24
greg.harveyAn old release of something like XAMPP would do it...
http://sourceforge.net/projects/xampp/files/
If someone has the chance to install one of those LAMPP or WAMPP installers from 2003, that'll be PHP4.
Comment #25
klonosI can do it, if only someone please specify in detail the tests that need to be done.
Comment #26
dwwPretty standard testing:
- Turn on E_STRICT
- Fetch available updates and see PHP notices being generated
- Apply the patch
- Re-fetch available updates and see no PHP notices
Thanks!
-Derek
Comment #27
deekayen CreditAttribution: deekayen commentedI know it's not PHP4, but the notices have gone away after applying this on Ubuntu 10.04. The available updates page also looks sane.
PHP 5.3.2-1ubuntu4.7 with Suhosin-Patch (cli) (built: Jan 12 2011 18:36:55)
Comment #28
VM CreditAttribution: VM commentedJust worked with this patch on PHP 5.3.3 in Ubuntu 10.10 and all seems to be working perfectly fine. I don't have PHP4 to test against.
I'll try to dig out an old laptop that I can throw PHP 4 on and see if this issue can't be moved forward.
Comment #29
anantagati CreditAttribution: anantagati commentedWorks good for on PHP 5.2.10-2ubuntu6.4 and Pressflow 6.20.97.
Comment #30
greg.harveyThere is no need for anyone else to report it works in PHP 5! It does! ;-)
See: http://drupal.org/node/1006938#comment-4188754
Comment #31
VM CreditAttribution: VM commentedI've been working on this for a portion of the day. I've made my way through versions of XAMPP up to 1.4.12 (2005) and can't get drupal installed to test the patch on PHP4 as of yet. I keep running into "notice: Use of undefined constant PHP_INT_MAX - assumed 'PHP_INT_MAX' in C:\xampp\xampp\htdocs\drupaltest\includes\database.inc on line 212." which stops me from getting to the configuration screen after the profile runs and installs the core modules.
I'll keep plugging away but thought I'd update on progress. If anyone has a suggestions on how to get around PHP_INT_MAX issue so as that I don't get too far away from minimal Drupal 6 requirements, I'd appreciate it.
Comment #32
tstoecklerIs there an easy way to install PHP4 on Ubuntu Maverick, i.e. a package repository where old packages are maintained or something?
Comment #33
deekayen CreditAttribution: deekayen commentedWhy not just grab an old ISO of Debian or Ubuntu for a VM instead of trying to backport a 5.x install?
Comment #34
greg.harvey@deekayen - that's not a bad idea either, but #31 is odd. It sounds as though something else has already broken PHP4 compatibility for Drupal 6, in which case this whole test is null and void.
I'm now sufficiently curious to bother creating a VM of an old Linux release.
Comment #35
mlee11111 CreditAttribution: mlee11111 commentedSubscribing.
Comment #36
DeFr CreditAttribution: DeFr commentedOk, so, yet another round of my terminal being flooded by this notice actually managed to trick me into installing a brand new VM running Ubuntu 6.06 to try to get the needed confirmation on PHP4.
First of all, it looks like support for PHP < 4.4 is already broken: according to http://php.net/manual/en/language.types.integer.php , PHP_INT_MAX has available "only" since PHP 4.4.0. This might be a problem in itself, given that INSTALL.txt still list PHP 4.3.5 as the minimum version, and its still what's defined by system.module . It was introduced in #499254: BIGINT handling, and probably broked pretty much everything for PHP < 4.4, since it now compares the value to the string 'PHP_INT_MAX', which will be true for all positive integers...
I've thus tested only in PHP 4.4, more precisely 4.4.2-1build1 (corresponding to http://packages.ubuntu.com/dapper/libapache2-mod-php4 ). What I did to setup the whole thing, in details :
At this point, I got exactly the same flood of notices and the big red warning that CCK 2.5 contained a security issue and should be updated as soon as possible, which was the expected output.
I then applied the patch, re-forced the update check, and this time, the notices were all gone, which sounded promising. CCK 2.5 was still marked as containing a security issue, which was good too.
I then upgraded CCK to 2.8, and the message correctly changed to a yellowish "That's not the recommanded version". After upgrading to CCK 2.9, all was green again. I haven't dealt with the update code much, but I guess this all means that both before and after the patch, D6 can still parse the informations update on PHP 4.4.2.
I would thus say that this patch is now tested in PHP 4.4.2 too, but I haven't done significant work in the VM, so if there's something else you'd like me to do to be super extra sure that it's still working, just ask.
Comment #37
dww@DeFr: Perfect, thanks! Wish we had gotten this in before 6.22, but I guess we can assume a 6.23 will happen at some point. ;)
Comment #38
dwwp.s. Sounds like we either need to reopen #499254: BIGINT handling or create a new bug about updating the min PHP requirements for D6.
Comment #39
DeFr CreditAttribution: DeFr commented@dww, was diving into the issue queue, and in fact, PHP_INT_MAX being undefined already has its own issue, #586468: Code uses a constant that is not defined in all version of PHP after version 4.3.5
Comment #40
dww@DeFr: perfect, thanks for cross-linking that here. So, all that remains here is a "commush" from Gabor for the patch at http://drupal.org/files/issues/555362-1.update_parse_files.D6.patch ;)
Comment #41
dpearcefl CreditAttribution: dpearcefl commentedI got bit by this bug when I did a clean D6 install over the weekend so I'd like to see it get into the dev codebase also, which it is not.
I don't see that the testbot has processed the patch yet. Perhaps the reason is explained in the issue handbook under the "Needs Review" status:
It looks like we may have skipped a step? The patch also needs to be attached to this ticket at the time the ticket goes to "needs review".
Comment #42
westwesterson CreditAttribution: westwesterson commentedUploading the same patch from above, in hopes it triggers test bot
Comment #43
klonosI am pretty sure that the test bot ignores D6 specific patches. More info/background here:
#1007210: Test bot should run tests against the core version the issue is set to.
#1171958: Allow files to be assigned to branch(es)/version(s) and thus tested against it (which I guess depends on #66484: Allow issues to be filed against multiple versions/branches.)
Comment #44
dpearcefl CreditAttribution: dpearcefl commentedSometimes the testbot ignores the patch if the filename is non-standard.
[description]-[issue-number]-[comment-number].patch
http://drupal.org/node/707484
You will need to resubmit the patch and change the status to needs review.
Comment #45
dwwThere's no automated test suite for D6 core. The test bot ignores all issues that are for the "6.x-dev" version. All patches for D6 core require manual testing. Thanks for your attempts, but really, there's nothing else we can do here except wait for Gabor to commit my patch http://drupal.org/files/issues/555362-1.update_parse_files.D6.patch which has now been confirmed tested on both PHP 5 and PHP 4.
Comment #46
mikeytown2 CreditAttribution: mikeytown2 commentedPatch #42 works!
Comment #47
paulgemini CreditAttribution: paulgemini commented#42 Works for me too!
Comment #48
stemount CreditAttribution: stemount commented(Patch #42)++
Comment #49
DeFr CreditAttribution: DeFr commentedFor the record, the patch posted in #42 is explicitely a repost of the first patch in this issue, which means that comment #7 and #30 still applies ; in short, the patch is known to work in PHP5, confirmation that it work in PHP4 was needed to get it commited though, which was given in #36 for PHP 4.4.2 . Obvisously, more reports that it work in PHP4 would be welcome, but all that's really needed here is to just wait for Gabor to comment and commit the patch, hopefully in his next round of reviews ;-)
Comment #50
klonosHow many more duplicates before this finally gets in?
...
#1009376: notice: Undefined index: in end() (line 257 of drupal-6\modules\update\update.fetch.inc).
#1021498: Remove E_NOTICE in update.fetch.inc
#1025344: $this->current_tag could be null in update_xml_parser->end() (line 257 of /modules/update/update.fetch.inc)
#1038704: Undefined index: in end() (line 257)
#1066232: Undefined index notices in update_xml_parser->end()
#1144952: Undefined index in update.fetch.inc on line 256
#1193184: Drupal Core's update module reports PHP Warnings
#1208330: Notice: Undefined index: in update_xml_parser->end() (line 256
#1241362: "Undefined index" error in update.fetch.inc
The list seems endless :/
Comment #51
VM CreditAttribution: VM commentedsomeone may want to ping gabor about this issue since it is specific to D6 ?
Comment #52
dww@klonos: Thanks for the list of duplicates...
@VM: Not sure why you downgraded from RTBC. Leaving this in that state is (in theory) the best way to get Gabor's attention. Plus, my original patch has been reviewed and tested (heavily) by the community, so RTBC is still the right status here. Anyway, sure, I'll also ping him out of band about this.
Comment #53
VM CreditAttribution: VM commentedThe switching of the status was unintentional. Must have been a slip of the mouse. My apologies.
Comment #54
Peter Bowey CreditAttribution: Peter Bowey commented#42 Works for (and is required) for Pressflow 6.22 too!
The fact that it NOT yet in Pressflow 6.22 surprises me - "a lot"!
"Hello' David Strauss" https://launchpad.net/~davidstrauss
Why is this 'FIX' not yet incorporated in D6 either?
The problem has been around for quite some time!
Comment #55
Gábor HojtsyNow committed to Drupal 6 core, so it will be part of the next Drupal 6 release and consequently (I assume) Pressflow as well. Thanks!
Comment #56
klonosFinally!!! Thanx Gábor.
Comment #58
theohawse CreditAttribution: theohawse commenteddon't see this in current pressflow distro, for those looking for this now, the following works for me with drush make:
This will throw an error hopefully when pressflow gets a new release.
Comment #59
webservant316 CreditAttribution: webservant316 commentedI was directed to this issue from drupal.org/node/1193184.
just got this error when running update.php
===
warning: array_merge() [function.array-merge]: Argument #2 is not an array in update.php on line 173.
warning: Invalid argument supplied for foreach() in update.php on line 337.
===
I am running Drupal 6.26 and was upgrading 7 modules: flag, print, smtp, spambot, token, ubercart, and user_import.
This seems like an old issue, why would I get this error?
Comment #60
webservant316 CreditAttribution: webservant316 commentedComment #61
DeFr CreditAttribution: DeFr commented@webservant316: #1193184: Drupal Core's update module reports PHP Warnings was erroneously marked as a duplicate of this bug, instead of being marked as a duplicate of #274283: Warnings on running update.php . In short, one of the update performed didn't return an array as it was supposed too.
Comment #62
webservant316 CreditAttribution: webservant316 commentedoops sorry and thanks!