When running the Drupal HTTP request test an exception in generated. That can be removed by adding @ to parse_url().

Since the code checks to make sure that it parsed the URL successfully and returns error messages anyway it would seem to make sense that the function would mask the errors triggered.

Updated test to include actual page fetch test.

CommentFileSizeAuthor
#20 drupal_http_request.test_2.patch1.11 KBmustafau
#12 drupal_http_request.test.patch3.94 KBboombatower
#10 295564-drupal-http-request.test.patch3.75 KBDamien Tournoud
#9 295564-drupal-http-request.test.patch3.3 KBDamien Tournoud
#7 drupal_http_request.test.patch1.62 KBboombatower
drupal_http_request.test.patch1.63 KBboombatower
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

obsidiandesign’s picture

obsidiandesign CreditAttribution: obsidiandesign commented

Is this a different approach/related to #283806: Notice appears when drupal_http_request called with an invalid URL? I found both when searching for information about the Drupal HTTP request test.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

I've commented on the other issue.

catch’s picture

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

Crtical since this is now our only remaining test with any kind of breakage, review forthcoming.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me to rely on our internal error handling and suppress the php warnings in the function call rather than just the test. This removes the exception, adds an extra test, lovely.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Reviewed & tested by the community » Needs work

Please we can do better than that:

+    $this->assertTrue(strpos($result->data, '<title>' . variable_get('site_name', 'Drupal') . '</title>') !== FALSE, t('Site title matches.'));
boombatower’s picture

boombatower CreditAttribution: boombatower commented

Meaning get rid of !== FALSE? The reason that is there is I'm not sure if assertTrue is type strict. If not that strpos returning 0 would fail even thought that is success.

Since the page should never have the as the first character we could consider that a fail anyway. I'll remove the !== FALSE.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Needs review
FileSize
drupal_http_request.test.patch1.62 KB

Updated, per #5-6.

boombatower’s picture

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

Still passes and already reviewed per #4.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Reviewed & tested by the community » Needs review
FileSize
295564-drupal-http-request.test.patch3.3 KB

I should have been clearer. I meant that we have a whole facility for asserting things on pages with DrupalWebTestCase, we should use it.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
FileSize
295564-drupal-http-request.test.patch3.75 KB

Oups, here is a new version that fixes an exception in DrupalWebTestCase::getUrl().

boombatower’s picture

boombatower CreditAttribution: boombatower commented

I realized that, but didn't use it since you can't without setting the content. Wasn't sure that was something we wanted to do.

I like what you've done, I'll take a look at it shortly.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs review » Reviewed & tested by the community
FileSize
drupal_http_request.test.patch3.94 KB

I like that you can set the content now, it has grown on me after thinking about it. I have added a more detailed description of the uses for drupalSetContent() and fixed a formatting issue related to the function indentation.

The test still passes and we both have reviewed this format.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

That means we are at 100% pass!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

YYYYAAAAAAAAAYYYYYYYYYY!!!!!!!!!

Best. Day. EVER! :)

mustafau’s picture

mustafau CreditAttribution: mustafau commented

The tests committed with this patch were reverted by the DB:TNG patch. See: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/tests/c...

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Fixed » Needs work
boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Reviewed & tested by the community

Doesn't it just need to be reapplied...not worked on?

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

Doesn't apply.

mustafau’s picture

mustafau CreditAttribution: mustafau commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal_http_request.test_2.patch1.11 KB

Re-roll.

boombatower’s picture

boombatower CreditAttribution: boombatower commented

Test still passes after applicate, as before...looks good.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Reviewed & tested by the community » Closed (duplicate)

edit: wrong issue.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Closed (duplicate) » Needs work

Code no longer applies.

mustafau’s picture

mustafau CreditAttribution: mustafau commented
Status: Needs work » Reviewed & tested by the community

Patch at #20 still applies.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

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

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