found a silly naming bug reading the aggregator entity code and it annoyed me enough to create a patch.

CommentFileSizeAuthor
#18 2638182-8.patch685 bytesAnonymous (not verified)
#17 2638182_aggregator.feed_form.patch697 bytesMixologic
#12 2638182-12-no-wai.patch1.63 KBAnonymous (not verified)
aggregator.feed_.form_.patch697 bytesAnonymous (not verified)
#8 2638182-8-updatf.patch697 bytesAnonymous (not verified)
#8 2638182-8-pdate.patch695 bytesAnonymous (not verified)
#8 2638182-8-updat.patch695 bytesAnonymous (not verified)
#8 2638182-8-ipdate.patch697 bytesAnonymous (not verified)
#6 2638182-6-__lulz__.patch701 bytesAnonymous (not verified)
#6 2638182-6-lulz.patch666 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

beejeebus created an issue. See original summary.

chx’s picture

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

OK , that is stupid indeed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, aggregator.feed_.form_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review

lol?

rerunning, because the entirety of this patch is:

diff --git a/core/modules/aggregator/src/FeedForm.php b/core/modules/aggregator/src/FeedForm.php
index aa5069c..3b87408 100644
--- a/core/modules/aggregator/src/FeedForm.php
+++ b/core/modules/aggregator/src/FeedForm.php
@@ -21,9 +21,9 @@ class FeedForm extends ContentEntityForm {
    */
   public function save(array $form, FormStateInterface $form_state) {
     $feed = $this->entity;
-    $insert = (bool) $feed->id();
+    $update = (bool) $feed->id();
     $feed->save();
-    if ($insert) {
+    if ($update) {
       drupal_set_message($this->t('The feed %feed has been updated.', array('%feed' => $feed->label())));
       $form_state->setRedirectUrl($feed->urlInfo('canonical'));
     }

Status: Needs review » Needs work

The last submitted patch, aggregator.feed_.form_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2638182-6-lulz.patch666 bytes
2638182-6-__lulz__.patch701 bytes

for shits and giggles, here are a couple more variations. this is *weird*, and may indicate some underlying CI oddity that needs to be figured out.

The last submitted patch, 6: 2638182-6-lulz.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2638182-8-ipdate.patch697 bytes
2638182-8-updat.patch695 bytes
2638182-8-pdate.patch695 bytes
2638182-8-updatf.patch697 bytes

lulzly times. so, $update fails, but $__lulz__ passes.

here are some more variations on the variable name.

ciss’s picture

ciss CreditAttribution: ciss commented
ciss’s picture

ciss CreditAttribution: ciss commented

Status: Needs review » Needs work

The last submitted patch, aggregator.feed_.form_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
2638182-12-no-wai.patch1.63 KB

hahahaha lolsob.

ciss’s picture

ciss CreditAttribution: ciss commented

FYI, tests that failed for the original patch with PHP 5.6, but not PHP 7:

  • Aggregator.Drupal\aggregator\Tests\AggregatorRenderingTest > updateFeedItems
  • Aggregator.Drupal\aggregator\Tests\DeleteFeedItemTest > updateAndDelete
  • Aggregator.Drupal\aggregator\Tests\DeleteFeedItemTest > updateFeedItems
  • Aggregator.Drupal\aggregator\Tests\FeedAdminDisplayTest > updateFeedItems
  • Aggregator.Drupal\aggregator\Tests\FeedFetcherPluginTest > updateFeedItems
  • Aggregator.Drupal\aggregator\Tests\FeedProcessorPluginTest > updateFeedItems
  • Aggregator.Drupal\aggregator\Tests\UpdateFeedItemTest > deleteFeed
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

i followed the steps at [#2487065] and got drupalci running locally (which is awesome stuff), but couldn't reproduce the fails.

i'll do some more poking locally, but i may need to move on to getting some time on a for-real drupalCI machine to figure out these fails.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

thanks to Mixologic for getting me set up on a DA testbot, i was able to reproduce the fails with aggregator.feed_.form_.patch.

the fatal errors are red herrings, because we are dead way before then:

[112124 - 1 - 1452367527.68] : [run-tests.sh 57] simpletest_script_run_one_test()  -->  [run-tests.sh 728] Drupal\simpletest\TestBase->run()  -->  [TestBase.php 1082] Drupal\aggregator\Tests\AddFeedTest->testAddFeed()  -->  [AddFeedTest.php 27] Drupal\aggregator\Tests\AggregatorTestBase->createFeed()
page => <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>404 Not Found</title>
</head><body>
<h1>Not Found</h1>
<p>The requested URL /checkout/aggregator/sources/add was not found on this server.</p>
</body></html>

so AggregatorTestBase->createFeed() is being told to GFY.

the apache logs look like this:

::1 - - [09/Jan/2016:19:41:13 +0000] "GET /checkout/user/login HTTP/1.1" 200 8361 "-" "simpletest570424;1452368473;56916259313e49.49984326;kCb2_xxEYHp369hUo0JmNoiQOsprsTvKijBw1G8s4Wo"
::1 - - [09/Jan/2016:19:41:13 +0000] "POST /checkout/user/login HTTP/1.1" 303 1041 "-" "simpletest570424;1452368473;56916259774cf0.13392925;BBbtpScmirMyXew071GuuNCobi90aPUe70cEY1FitrQ"
::1 - - [09/Jan/2016:19:41:13 +0000] "GET /checkout/user/2 HTTP/1.1" 200 7476 "-" "simpletest570424;1452368473;56916259995358.19205968;Uj6fKVXhn7xhtfjxz_J2wAB_q12t6yZlrZkU2a9uyJM"
::1 - - [09/Jan/2016:19:41:13 +0000] "GET /checkout/aggregator/sources/add HTTP/1.1" 404 389 "-" "simpletest570424;1452368473;56916259c14162.38537887;r8QJRLWs1pQDnTAl9tBaWHcKKMlzAYLUpgfCvtInO-4"
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

JFC.

with a lot of help from Mixologic, we found it.

[pid  8337] stat("/var/www/html/checkout/aggregator.feed_.form_.patch", {st_mode=S_IFREG|0664, st_size=697, ...}) = 0
[pid  8337] getdents(15, /* 0 entries */, 32768) = 0
[pid  8337] close(15)                   = 0
[pid  8337] gettimeofday({1452391473, 87084}, NULL) = 0
[pid  8337] gettimeofday({1452391473, 87167}, NULL) = 0
[pid  8337] write(7, "[Sun Jan 10 02:04:33.087167 2016] [negotiation:error] [pid 803] [client ::1:53180] AH00687: Negotiation: discovered file(s) matching request: /var/www/html/checkout/aggregator (None could be negotiated).\n", 204) = 204
[pid  8337] read(14, 0x7fef0dd42048, 8000) = -1 EAGAIN (Resource temporarily unavailable)
[pid  8337] gettimeofday({1452391473, 87462}, NULL) = 0
[pid  8337] writev(14, [{"HTTP/1.1 404 Not Found\r\nDate: Sun, 10 Jan 2016 02:04:33 GMT\r\nServer: Apache/2.4.7 (Ubuntu)\r\nContent-Length: 229\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n", 160}, {"<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">\n<html><head>\n<title>404 Not Found</title>\n</head><body>\n<h1>Not Found</h1>\n<p>The requested URL /checkout/aggregator/sources/add was not found on this server.</p>\n</body></html>\n", 229}], 2) = 389
[pid  8337] gettimeofday({1452391473, 87652}, NULL) = 0
[pid  8337] write(9, "::1 - - [10/Jan/2016:02:04:33 +0000] \"GET /checkout/aggregator/sources/add HTTP/1.1\" 404 389 \"-\" \"simpletest665834;1452391473;5691bc311467a0.66573945;BrXIx17ULctgUAjXaJgwyn51l56BxRKcGqoYHTNQW3o\"\n", 195) = 195

so the patch file is interfering with aggregator module paths. ffs. i hate computers.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
FileSize
2638182_aggregator.feed_form.patch697 bytes

Here's the original broken patch with a non-conflicting filename. We'll fix this in drupalci in the future.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2638182-8.patch685 bytes

dawehner asked for SAVED_UPDATED, new patch uses that.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thank you for that! Small improvements over time matter

catch’s picture

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

Committed/pushed to 8.1.x, thanks!

  • catch committed 19e52dc on 8.1.x 
    Issue #2638182 by beejeebus, Mixologic: fix badly named variable in...

Status: Fixed » Closed (fixed)

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