The most obvious one that I've found so far is that uploading files using the @ trick is now deprecated in favor of CurlFile(), easy to fix. That is an E_DEPRECATED, which is missing in Simpletests error map, so it blows up because the group is empty.

See also #1867192: Testbots need to run on 5.4, 5.5, 5.6 and 7

For reference, this notice is thrown:

Deprecated: curl_setopt(): The usage of the @filename API for file uploading is deprecated. Please use the CURLFile class instead.

CommentFileSizeAuthor
#20 php55-curl-file-upload-2054205-20.patch1.4 KBpfrenssen
#20 interdiff.txt873 bytespfrenssen
#18 php55-curl-file-upload-2054205-18.patch1.39 KBpfrenssen
#18 interdiff.txt1.38 KBpfrenssen
#9 php55-curl-file-upload-2054205-9.patch1.05 KBberdir
#1 php55-curl-file-upload-2054205-1.patch1.99 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new php55-curl-file-upload-2054205-1.patch1.99 KB

Here's a patch for that. Haven't found anything else yet that's broken except those that are broken on 5.4 too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ok, found something weird. NodeEntityFieldQueryAlterTest is a test without any test methods, that throws a fatal in the CLI runner and shows now results page in the UI.

Doesn't seem to be 5.5 specific?

Crell’s picture

Crell commented

I can't help but wonder why we're still using manual curl in Simpletest instead of Guzzle...

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

@Crell I don't think anyone is spending time trying to refactor SimpleTest at the moment.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

I have verified that the new constants are available since 5.3.0 so that's good. And http://php.net/curl_file_create looks good too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Opened #2056293: Remove empty test Drupal\node\Tests\NodeEntityFieldQueryAlterTest for the empty test class.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I've run the two File upload tests without and without this patch on 5.4 and 5.5 and can confirm we currently have fail on 5.5 in HEAD and that the patch resolves the issue.

The changes look good. Committed 7f02544 and pushed to 8.x. Thanks!

David_Rothstein’s picture

David_Rothstein commented
Version: 8.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

I think this affects Drupal 7 too, but not sure this qualifies as critical for Drupal 7... at least not yet.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new php55-curl-file-upload-2054205-9.patch1.05 KB

Yes, makes sense.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Reviewed & tested by the community

Excellent, thanks, RTBC!

Crell’s picture

Crell commented

#9: php55-curl-file-upload-2054205-9.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

No longer applies

git ac https://drupal.org/files/php55-curl-file-upload-2054205-9.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1080  100  1080    0     0   1346      0 --:--:-- --:--:-- --:--:--  1465
error: modules/simpletest/drupal_web_test_case.php: does not exist in index
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community

@alexpott, uh you confused me for a second ;) *You* don't apply to this issue :)

This is a 7.x backported, you already committed the 8.x patch a while ago :p

And, the patch still applies fine :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work

E_DEPRECATED and E_USER_DEPRECATED only exist since PHP 5.3, Drupal 7 should run on 5.2 as well. You could argue that we only make this change in the web test file, but I think a lot of users will still run their automated build tests on 5.2, too.

Or is there a policy that we abandoned 5.2 for D7 now?

pfrenssen’s picture

pfrenssen
Location Sofia
commented

Indeed Drupal 7 still officially supports PHP 5.2.5 and higher, even though PHP 5.3 is recommended. I suppose we can simply define those constants ourselves if they do not exist. They will never be thrown on PHP 5.2, but they will generate undefined constant notices.

I have looked into potential problems with bitwise operations with these constants and E_ALL which has different values on different PHP versions. Most of the times when this is used it is to subtract a value from E_ALL (eg E_ALL & ~E_USER_DEPRECATED) and this will give correct results, even on PHP 5.2.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think we can simply add those two to the array conditionally if they exist. We don't care if they exist or not, we only care that when they are thrown, the test doesn't explode.

pfrenssen’s picture

pfrenssen
Location Sofia
commented

Marked #2101235: DrupalTestCase::errorHandler() does not accept E_DEPRECATED or E_USER_DEPRECATED (introduced in PHP 5.3) as a duplicate of this issue.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new interdiff.txt1.38 KB
new php55-curl-file-upload-2054205-18.patch1.39 KB

Added the constants conditionally. Also restored the comment from the D8 patch that was lost in the backport to D7.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
+++ b/modules/simpletest/drupal_web_test_case.php
@@ -2042,7 +2051,14 @@ class DrupalWebTestCase extends DrupalTestCase {
+                // Use the new CurlFile class for file uploads when using PHP
+                // 5.5.0.

why 5.5.0? Also applies to any version higher than 5.5.0? Just use "5.5" as in the original patch or "5.5 or higher".

Otherwise looks ready.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt873 bytes
new php55-curl-file-upload-2054205-20.patch1.4 KB

Fully agree!

chx’s picture

chx commented
Status: Needs review » Needs work 
+      if (defined('E_DEPRECATED')) {
+        $error_map += array(
+          E_DEPRECATED => 'Deprecated',

I bet you'd meant !defined

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Reviewed & tested by the community

No, the patch is correct. If the constant is defined we add them to our error map of known error codes, otherwise we are on PHP 5.2 and they don't exist anyway.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Included the notice that is thrown to help people find this issue.

heine’s picture

heine commented

Actual notice on PHP 5.5 is "Notice: Undefined offset: 8192 in /var/www/webroot/modules/simpletest/drupal_web_test_case.php on line 545"

Adding for search.

Crell’s picture

Crell commented

20: php55-curl-file-upload-2054205-20.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

20: php55-curl-file-upload-2054205-20.patch queued for re-testing.

David_Rothstein’s picture

David_Rothstein commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/001532a

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +7.25 release notes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.