Length of Trimmed Description

same patch work for both D7 & D8.

hi,

patch 1045786.patch only works around the issue of being able to select the length of trimmed description. it doesn't actually trim the content

thanks,
njardim

ahh I can see what you means. it maybe by design, looking for more comment.

if we need to add it in, then may need to add a new filter for aggregator too.

hi droplet,

we really need to trim the feeds contents, some feeds are just to long for displaying

thanks,
njardim

maybe this can help:

http://api.drupal.org/api/drupal/includes--unicode.inc/function/truncate...

just need to know where to call this from to display the trimmed feeds

regards,
njardim

as I can tell, text_summary is the best for trim contents. (safety for HTML tags)

any news regarding this issue?

thanks,
njardim

test for the aggregator settings page

Thanks for the patches! Reading this test, it's not clear to me that it will fail without the bug fix you provide in #9. Does the test fail locally when you do not apply the fix? If it does fail locally, could you upload the test in a patch by itself to demonstrate that it fails without the bugfix?

Also, here are a couple minor notes about the code documentation:

+++ b/modules/aggregator/aggregator.testundefined
@@ -278,6 +278,34 @@ EOF;
+   * Submit settings page, ensure all form element has correct default value.

I'd change this to: "Tests the settings form to ensure the correct default values are used."

+++ b/modules/aggregator/aggregator.testundefined
@@ -278,6 +278,34 @@ EOF;
+class AggregatorConfigurationTestCase extends AggregatorTestCase {

We need a documentation block for this class. See http://drupal.org/node/1354#classes for more information.

Finally, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Here are the rerolled and updated patches.
a test only patch too, btw the original test only patch is here: http://drupal.org/node/1324102#comment-5183936, tihs is the same patch, but the issue is a duplication of this.

oh, sorry, wrong patch...
(test only was correct)

+++ b/core/modules/aggregator/aggregator.testundefined
@@ -278,6 +278,37 @@ EOF;
+ * Test aggregator configuration settings.

This works too, but it should be "Tests..." :) Other than that, this looks good to me. Waiting to see what testbot says.

oh, my... :)

Note that there is a testbot issue at the moment, which is why the patch failed. We'll want to re-test it once that is fixed.

Alright, the revised patch in #15 looks ready to me.

Committed/pushed to 8.x, thanks!

Looks like this should be backported to 7.x.

Backported to D7

Tests pass, looks good.

+++ b/core/modules/aggregator/aggregator.test
@@ -278,6 +278,37 @@ EOF;
+    $this->drupalPost('admin/config/services/aggregator/settings', $edit, t('Save configuration'));
+
+    foreach ($edit as $name => $value) {
+      $this->assertFieldByName($name, $value, t('"@name" has correct default value.', array('@name' => $name)));
+    }

In case of a form validation error, the drupalPost() ends up on the same page on which the original form is shown, containing the values that were attempted to be submitted ($edit), and settings have not been saved yet.

Thus, there's a chance that the assertFieldByName() assertions will wrongly succeed. You may overcome this by asserting that the "The configuration has been saved." message appears on the page after the drupalPost().

Sun, just to be clear do you think we have to add something like

$this->assertMessage("The configuration has been saved.");

first in D8 or do you prefer it to be included in the D7 patch.

So do we need a followup patch/NW for D8 to correct the test?

Alright, sun suggested I "be bold," so here it is.

Let's create a followup patch for D8, a new patch rolled against current HEAD. Something like the code @aspilicious suggests should work fine; just add it right after the drupalPost().

Then, create a D7 backport patch that includes both the original patch and that addition. You can name the D7 patch my_patch_name-d7.patch to keep the testbot from testing it while the issue is filed against D8.

Tagging novice for the above. :) Thanks in advance!

D8 patch updated per xjm's request. D7 patch to follow.

My first *possible* core patch. Excited!

+++ b/core/modules/aggregator/aggregator.testundefined
@@ -308,6 +308,7 @@ class AggregatorConfigurationTestCase extends AggregatorTestCase {
+    $this->assertText('The configuration options have been saved.'); //, $message = '', 'other');

The last part needs to be removed.

the // and everything behind it.

-10 days to next Drupal core point release.

Ah, got too excited. Fixed!

asserText needs a translated string,(see: http://drupal.org/node/265828)

so the correct addition is:

$this->assertText(t('The configuration options have been saved.'));

Yeah... I did a grep through core and most of the assert text messages are packed in a t() function. So yeah..

Thanks for the catch Désiré! Updated D8 module and backported D7 attached.

Looks good to me. Thanks @Albert Volkman!

Committed/pushed to 8.x, back to D7 again.

D7 patch available in #33.

Re-uploading the patch in #33 without the -D7 at the end so testbot tests it.

Committed and pushed to 7.x. Thanks!

is there a link to the entire updated aggregator core module that i might simply replace versus applying this patch? i'm not sure how to apply this patch, but this is precisely the issue i've been encountering (trimmed length)

This fix will be included in the next release of Drupal. Short of waiting until then, your only option currently is to use the supplied patch.

thanks. just used text mate to edit that one line and committed. oddly, when i save to trimmed length 200, it saves on admin, but when i view actual aggregator category with all items, it's still showing long entries...