Background: part of the task to update the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- review if the text in aggregator_help() is up to date for Drupal 8
- test the links embedded in the text
- update d.o. docs at http://drupal.org/node/289

Comments

batigolix’s picture

Issue summary:View changes

formatting

mrP’s picture

Status:Active» Needs review

- The text of aggregator_help is accurate for Drupal 8.
- All embedded links are working.
- http://drupal.org/documentation/modules/aggregator and http://drupal.org/node/774856 updated.

jhodgdon’s picture

Status:Needs review» Fixed

Thanks! I guess we can close this issue then.

batigolix’s picture

Status:Fixed» Needs work

The help text links to a page that seems to be dead:

      $output .= '<dt>' . t('OPML integration') . '</dt>';
      $output .= '<dd>' . t('A <a href="@aggregator-opml">machine-readable OPML file</a> of all feeds is available. OPML is an XML-based file format used to share outline-structured information such as a list of RSS feeds. Feeds can also be <a href="@import-opml">imported via an OPML file</a>.', array('@aggregator-opml' => url('aggregator/opml'), '@import-opml' => url('admin/config/services/aggregator'))) . '</dd>';

The url('aggregator/opml') part results in a broken link.

batigolix’s picture

Component:aggregator.module» documentation

changing component

batigolix’s picture

Correction: the url "aggregator/opml" isnt actually a broken link. Drupal is supposed to show an opml file there, but the file is not generated correctly. It seems to generate the OPML but HTML is appended to it.

batigolix’s picture

Component:documentation» aggregator.module
Category:task» bug

Changing the component to see if this a bug, a broken reference in the help text or just a brain flaw

To reproduce:
- enable aggregator
- on /admin/config/services/aggregator add a feed (e.g. http://drupal.org/rss.xml)
- on /admin/help/aggregator click "machine-readable OPML file"

jhodgdon’s picture

Let's file that as a separate issue and go back to dealing with the help file here.

ifrik’s picture

Links need to be formated following:
https://drupal.org/node/632280#url-note

lostkangaroo’s picture

Issue tags:+Help text
StatusFileSize
new7.39 KB
PASSED: [[SimpleTest]]: [MySQL] 60,331 pass(es).
[ View ]

The problems from #3 are the result of a missing route which is solved in #2039277: Convert aggregator/opml to the new controller style..

As such to not break Drupal that link has been untouched but all others are updated as well as a depreciated markup tag.

lostkangaroo’s picture

Status:Needs work» Postponed

awaiting the results of #2039277: Convert aggregator/opml to the new controller style. to finish help text link update

lostkangaroo’s picture

Issue summary:View changes

added task

ParisLiakos’s picture

Issue summary:View changes
Status:Postponed» Needs review

this is a route now

Status:Needs review» Needs work

The last submitted patch, 9: drupal8.aggregator-module.1977608-9.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Needs review
lostkangaroo’s picture

Status:Needs review» Needs work

Noticed a few things looking back over the patch.

jhodgdon’s picture

I chatted with lostkangaroo today in IRC. Here are some things that need fixing in this patch:

a) Link to online help doesn't follow our standards (as to what is in the link text) - https://drupal.org/node/632280

b) A few of the links need title attributes for accessibility (if the link text is not descriptive of where the link goes, it needs a title). Example: <a href="!aggregator-sources">their source</a>. Please check and see if there are others.

c) Still a few links need to be converted to https://drupal.org/node/632280#url-note -- should all use Drupal::url().

d) As discussed today in IRC, in the OPML section, we'd like to:
- Describe what OPML is
- Tell where to go to import OPML
- Tell where to go to download OPML (which is on the aggregator/sources page -- it's the feed icon on that page).
- We probably do not need a link in the help to the OPML download itself.

e) In the cron section, link to the system cron settings page in the site instead of to drupal.org.

f) Any links to drupal.org need to be https:

Thanks!

lostkangaroo’s picture

StatusFileSize
new8.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,930 pass(es).
[ View ]
new6.66 KB

There actually does not seem to be a reliable method of obtaining human friendly opml information other than the link mentioned in #15. That appears to head to the actual opml machine output when used in conjunction with the latest patch from #2039277: Convert aggregator/opml to the new controller style.. It almost seems as if a new config page would be needed to list the urls/files available for opml as these are all that are required to import or export this data. With this in mind I did not include information on where or how to obtain it in this patch.

lostkangaroo’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Status:Needs review» Needs work

Thanks! Better! A few items to address:

a) Regarding OPML output, I thought I saw something on the sources page that links to the OPML output? Let's see, yes:

    $build['feed_icon'] = array(
      '#theme' => 'feed_icon',
      '#url' => 'aggregator/opml',
      '#title' => $this->t('OPML feed'),
    );

This in AggregatorController.php.

Other comments on the patch:

b) The change removing the - in "RSS-, RDF-, and Atom-based feeds" -- those actually are correct, can you put them back?

c) Thanks for adding link titles! However I don't think "link to original content of the feed" is correct for the aggregator/sources page. It seems, instead, to be a list of the feed sources? Also it is not necessary (or even a good idea) to put "link to" in a link title.

d) We need to make sure that when the help file refers to specific pages or specific things to click/see on pages, the text in the help file matches what a user would see in the UI. Most of this looks right, but could you check on the Blocks information? I think there is not a block called 'latest category'? (that's in the admin/config/services/aggregator section).

jibran’s picture

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new7.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,981 pass(es).
[ View ]

Re-rolled patch

jhodgdon’s picture

Status:Needs review» Needs work

I don't think the change in the      case 'admin/config/services/aggregator/add/opml': section is correct? Well, I don't know, but why change from an acronym tag to abbr tag?

It also doesn't look like the comments in #18 have been addressed.

batigolix’s picture

No, the comments in #18 have not been addressed. I only re-rolled the patch because I had the intention to work on it.

batigolix’s picture

Status:Needs work» Needs review
Parent issue:» #1908570: [meta] Update or create hook_help() texts for D8 core modules
StatusFileSize
new6.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,905 pass(es).
[ View ]
new6.65 KB

New patch.

Regarding #18:

a) Link to opml feed restored (it works! -- see #6)
b) Hyphens restored
c) Title attribute removed
d) I restored the original text under case "admin/config/services/aggregator" case 'admin/config/services/aggregator/add/feed' case 'admin/config/services/aggregator/add/opml' and changed the tokens and url(). I changed the reference to the block. The block is called Aggregator feed in the UI. Not sure if the wording is clear, though

Regarding #21:

- I restored the original text under case 'admin/config/services/aggregator/add/opml'

lostkangaroo’s picture

We should take into account that categories have been removed entirely from Aggregator with #2127725: Remove category handling from aggregator. This really changes things.

batigolix’s picture

Ill check what consequences this has for the help text ...

batigolix’s picture

Status:Needs review» Needs work

There is a status for that

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 59,973 pass(es).
[ View ]

Removed the references to categories.

jhodgdon’s picture

Thanks! I think that the main module help is now in pretty good shape.

There are still a couple of issues with the help on admin/config/services/aggregator:

a) Possibly the URLs that describe Atom and the other feed formats should be put directly into the translatable text, because they are pointing to English-language pages. A translator might want to substitute a page in a language their users could actually read? At least with Wikipedia links they could maybe find a translation, but these pages, I'm not sure.

b) On the second line of that section:

!block' => url('admin/structure/block')))

This call to url() needs to be converted.

jhodgdon’s picture

Status:Needs review» Needs work
lostkangaroo’s picture

I still don't feel like the link to the OPML generated page is more detrimental than helpful but at this point all other solutions might be lost with the removal of categories and the simplification of the admin page. Either way at this point I give up the argument for its removal.

InternetDevels’s picture

Status:Needs work» Needs review
StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,871 pass(es).
[ View ]
new4.9 KB

Fixed issues from comment #28.

jhodgdon’s picture

Issue tags:+Needs manual testing

RE #30 - We aim to document what the module does... why do you think the link to the OPML page should be omitted? It seems like useful information about something that the module does?

Anyway... aside from that point, I think this is looking good and it's probably time for a manual test. On both the module Help page and the admin pages for the module:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

lostkangaroo’s picture

My main issue with the direct link to the OPML page is that I believe that the OPML generated content should be accessible through the main admin page for this module through an export action. In the help we should be providing information on how to export or access exported OPML content rather than directly linking to it. This is because of the machine generated output causing confusion in many ways as most if not all other modules link to admin pages to do this type of operation.

The problem before was there was no easy way to accomplish this due to the category system creating multiple OPML outputs, and the only way to find it was to find an icon at the end of sometimes huge lists. Creating the instructions to find it would prove difficult as the icon might not be in the same place or even the same icon due to theming. Since the categories are removed now and any hope to reimplement them moved to contrib it should be safe to move the link to an export action next to the import button already existing.

Either way the manual test is complete with all existing elements tested, my concerns with the existing patch are noted below.

  • Should have noticed this earlier but I think abbr is correct since acronym is deprecated HTML. Its seems inconsistent also with its useage on certain pages and not others specifically for OPML.
  • Should we link this text "imported via an OPML file" directly to the OPML import administration page?
jhodgdon’s picture

The acronym vs. abbr tag -- you are correct, sorry I said the wrong thing earlier:
http://www.w3.org/TR/html5/text-level-semantics.html#the-abbr-element

Sure on the OPML import link.

And if the module changes so that OPML exports are different from how they are now, hopefully the help will be updated... but meanwhile, it still seems like a good idea to talk about it in the help? If you can think of a better thing to say, let's say it?

lostkangaroo’s picture

StatusFileSize
new3.58 KB
new7.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,874 pass(es).
[ View ]

Changed existing OPML acronym tag to abbr and added one more to the main help page.
Changes OPML import link to the import OPML page

I have tested the new link locally with no problems and the new abbr tag appears as it should. I am thinking this should be good to go.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing

OK, works for me. Thanks!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 5905ad6 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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