Backport the Announcements Feed core module to Drupal 7

Ok. Let me get somebody on this and get a first cut done and get back.

I was involved in the later stages of the module for D10. I will try to review and test this tomorrow.

First of all, thanks so much for doing all this!

I did a first pass at reviewing all the code and left comments in the MR. A lot of the comments are related to features that were removed in the last stages, so it seems that the port was based on a non-final version of the module. I added references to the D10 module that hopefully will help.

Thanks @fjgarlin
I have removed the announce-new implementation from D7 code as per latest version of D10 and added some CSS to make appearance better.
I am working on fixes of comments in MR.

Fixed many MR reviews and working on remaining feedback.

Great work! I'll wait until you mark it as "Need review" to do another review round and then resolve all the threads that are actioned.

The Merge request reviews and feedback have been fixed.

@fjgarlin Kindly check and let me know the feedback.

Thanks @mitthukumawat for working on this!

I have checked the new changes only briefly, but there still seems to be a lot of coding style issues, PHP 5.3 compatibility issues and some unresolved comments from @fjgarlin (like the untranslated string). Unfortunatelly we do not have such strict code style checking in tests as D10 has, but we need to resolve these issues - missing semicolons, indentation, short array syntax usages, ... It would be great if you can check PHP 5.3 and PHP 8.2 test results (see merge request, I have triggered the test runs) and resolve these issues so that also these tests are green. You can use for example the coder module to check the codestyle.

I am not personally happy with the naming which is used in various files - module is named "Announcements Feed", tests are named "Announce feed test", variable names without the module prefix, cache table name, etc.. We need to unify this, so it is not confusing for users. Please just stick with everything as close as possible to "Announcements Feed" expression / name.

100% agree with the above. Thanks for looking at it @poker10.

I will go through the MR and try to resolve what I can, but the above points stand and need to be resolved.

Making a summary of them here:
* Fix PHP5.3 issues
* Fix PHP8.2 issues
* Fix coding standard issues (semicolons, short array syntax, indentation...)
* Make sure all hardcoded strings that are shown in the UI are translatable
* Consistency in the usage of the name all over (announcements_feed / Announcements Feed)

I realized I cannot resolve my own threads (due to this gitlab issue), so rather than commenting again, I just added a checkmark (✅) to the ones that I consider solved, and perhaps somebody with enough permissions can resolve them for me.

I have marked all threads with checkmark as resolved. Seems like 8 unresolved comments were left.

I have fixed many MR feedback. Also fixed below from above list of issues mentioned in comment #17 and #18.
* Fix coding standard issues (semicolons, short array syntax, indentation...)
* Make sure all hardcoded strings that are shown in the UI are translatable
* Consistency in the usage of the name all over (announcements_feed / Announcements Feed)
I am working on the PHP5.3 and PHP8.2 issues and some unresolved MR feedback.

Fixed the Merge request reviews and additional feedback along with test cases and php version 5.3 and 8.2 issues.
I have modified the code as per best compatibility for these php version.
Please review new changes and mark resolve if new changes found as per expectations.

Quickly went through it and added some nits/feedback from my side :)

Thanks @BramDriesen for your feedback.
I have fixed the suggested modifications in latest commit.

Some more remarks 😉

Can we please match the function comments / other text as much as possible with the D10 code? I see big difference in various comments + frontend/backend strings and it is a bit hard to do a proper review. I am not sure why the comments / strings are not a copy/paste from D10 (where possible), but if was not possible somewhere, it needs to have at least the same meaning. One example:

D10:

  /**
   * Fetches the feed either from a local cache or fresh remotely.
   *
   * The feed follows the "JSON Feed" format:
   * - https://www.jsonfeed.org/version/1.1/
   *
   * The structure of an announcement item in the feed is:
   *   - id: Id.
   *   - title: Title of the announcement.
   *   - content_html: Announcement teaser.
   *   - url: URL
   *   - date_modified: Last updated timestamp.
   *   - date_published: Created timestamp.
   *   - _drupalorg.featured: 1 if featured, 0 if not featured.
   *   - _drupalorg.version: Target version of Drupal, as a Composer version.
   *
   * @param bool $force
   *   (optional) Whether to always fetch new items or not. Defaults to FALSE.
   *
   * @return \Drupal\announcements_feed\Announcement[]
   *   An array of announcements from the feed relevant to the Drupal version.
   *   The array is empty if there were no matching announcements. If an error
   *   occurred while fetching/decoding the feed, it is thrown as an exception.
   *
   * @throws \Exception
   */
  public function fetch(bool $force = FALSE): array {

D7:

/**
 * Helper function to fetch the announcements feed.
 *
 * Return all announcements from the feed relevant to the Drupal version.
 * Structure of the JSON feed:
 *   id: Id.
 *   title: Title of the announcement.
 *   teaser: Announcement teaser.
 *   link: URL
 *   date_modified: Modified date of announcements.
 *   date_published: Published date of announcements.
 *   version: Drupal version of announcements.
 *   featured: 1 is the announcement is featured..
 *
 * @return array
 *   An array of Announcements from the feed, or an empty array if the feed
 *   is empty or an error in fetching/decoding feed.
 *
 * @throws Exception
 */
function announcements_feed_fetch() {

You can see big difference in "date" vs "timestamp" for example (but also other fields):

 *   date_modified: Modified date of announcements.

   *   - date_modified: Last updated timestamp.

Another example are texts in the announcements_feed_help(), which are different comparing to the D10 version.

Difference is also in the announcements_feed_permission(), another are in announcements_feed_is_relevant_item() and announcements_feed_validate_url() comments. There is a difference also in the main module definition in the announcements_feed.info.yml vs announcements_feed.info file, etc..

-------------

A lot of people worked on the D10 module and it was multiple times carefully reviewed (by the UX experts, core committers, ...). Therefore we need to stick with frontend texts, comments and other stuff as close as possible to the D10 version - because that is how it was approved and it does make sense (of course if the D7 module is different in something, it is possible to diverge, but that will be minimum of things). Thanks!

Regarding the new bell icon, which will appear in the admin toolbar and the position of the Announcements link there, I think this will need another separate usability review, because this is a completely new element not used in D7.

Thanks @poker10 for your feedback and @jijojoseph_zyxware for fixing the comments. These seems fine to me.

NW At least for the release note and change record.

Has this been tested WITHOUT the "toolbar" module enabled?
See the related D10 issue: #3356515: Give users a way to access announcements if toolbar module is disabled which is already RTBC.

There is a hook_menu() defined which adds an admin/announcements_feed item. Can't remember from the top of my head if that's sufficient for an item to appear in the configuration pages. Been a long time since I've touched D7 😁

https://git.drupalcode.org/project/drupal/-/merge_requests/3965/diffs#ef...

I just tested and yes, it shows up under the /admin links when the toolbar module is disabled.
I also marked with ✅ the threads that were still opened by me meaning that the thread can be resolved.

I guess we can ping the right people to review what's next. Do we have a person we can tag to review each of the following?
- Usability review
- Product manager review
- Framework manager review

And I'm not sure what's the process to follow on the change record and the release note.

I have no clue on who we can tag. I guess once we set it to Needs Review and post a link to this issue in the #needs-review-queue-initiative chat in Slack someone will pick it up.

As for the release note, I copied the snippet from the D8/9/10 issue. I will do the same for the change record and adjust where needed.

Drafted change record: https://www.drupal.org/node/3362227

Guess we can set it to needs review once more.

Putting down responsible persons for D7, but please correct me if I am wrong.

- Usability review
I am not sure if the Usability meetings (#ux on slack) covers also D7 issues, but hopefully yes, so we can ask them for the review of the placement of the new link with the icon, because it is different in D7 vs D10 (if there won't be a new icon, I suppose this wouldn't be needed).

- Framework manager
This should be Fabianx.

- Product manager
I think this should probably be Gabor Hojtsy.

- Releaser manager
This should be mcdruid.

Regarding the change record and release notes, probably the change record draft and release notes snippet in the issue summary will be enough in this phase. (done)

Thanks!

The progress looks good, thanks all! I have done a more in-depth review and posted some comments to the MR.

Here are some more questions/points:

1. I think we are completely missing the cron part where the announcements are downloaded periodically, see D10 code:

/**
 * Implements hook_cron().
 */
function announcements_feed_cron() {
  $config = \Drupal::config('announcements_feed.settings');
  $interval = $config->get('cron_interval');
  $last_check = \Drupal::state()->get('announcements_feed.last_fetch', 0);
  $time = \Drupal::time()->getRequestTime();
  if (($time - $last_check) > $interval) {
    \Drupal::service('announcements_feed.fetcher')->fetch(TRUE);
    \Drupal::state()->set('announcements_feed.last_fetch', $time);
  }
}

We need to add two new variables (announcements_feed_last_fetch and announcements_feed_cron_interval), implement the hook_cron() and update the announcements_feed_fetch() with the $force parameter (see D10).

----------

2. I am not a front-end expert, but I am curious why we have not used UL/LI approach in the template file? As the same is used in most admin pages like "structure", "configuration", "reports", etc. Instead there is a custom styling using DIVs.

----------

3. We are creating a new cache table - cache_announcements_feed. However it seems to me that this table will have only zero or one row at any moment. If this is correct, then I think we should use the default cache table instead and save outself from creating a new table. See:

cache_set('announcements_feed', $announcements, 'cache_announcements_feed', REQUEST_TIME + variable_get('announcements_feed_max_age'));

----------

And a few notes for other reviewers:

1. Not sure why the announcements links do not have target="_blank", but it seems the same way as it is in D10, so probably we are good there.

2. In D10 the toolbar item is named "Announcements" (the same as in this patch), but the title of the page admin/announcements_feed is "Community announcements". We probably cannot do much about this, because in D7 the hook_menu() item title is used automatically in the menu as well. So unless we want to use a drupal_set_title() in the announcements_feed_get_announcements(), we can probably keep it as it is.

----------

I have not checked the tests yet (codestyle and comparision with D10), but let's fix these issues first. Thanks!

I have marked all threads with checkmark and other fixed threads as resolved.

Thanks all for your feedback. I have resolved these as per comments and suggestions in MR.
Also as per #38, I have looked into all points and added fix for those as below :

1. Added the hook_cron() in module.
2. Change the theme to appear as ul and li for announcements same as other admin listing pages.
3. Removed the separate cache table for announcements feed and used default cache with cid as announcements_feed

.

4. Added target="_blank" for hyperlinks of announcements, Learn more and View all announcements buttons. I have added these as per an administrator perspective as they will not want to leave site instead the announcements will open in a new tab.
5. Added the page title as Community announcements using drupal_set_title() in announcements_feed_get_announcements().

@poker10
I have added the proper phpdoc for these functions.

announcements_feed_fetch()
announcements_feed_get_announcements()
announcements_feed_get_all_announcements()

Please review the changes in MR and mark resolved if it seems correct.

Thanks @mitthukumawat for your work here! Checked the comments and almost all are done. Added a few small nitpicks regarding the new changes.

Re:

Added target="_blank" for hyperlinks of announcements, Learn more and View all announcements buttons. I have added these as per an administrator perspective as they will not want to leave site instead the announcements will open in a new tab.

I mentioned this as a point to consider, but OK. However this would need an additional review, because D10 only has target="_blank" on the View all announcements link (not sure about the exact reason).

We would need to review the error-throwing as well, I have not tested all possible combinations. And as mentioned, I still have not looked at the tests, so if anyone else wants to review that carefully, feel free to do it :) If not, I will try to find some time for this as well.

I am adding the usability review (requested in #29) to the list of Remaining tasks in the issue summary.

Can the other items on that list be marked as complete (by crossing them out)?

I have updated the Remaining tasks section and added some other reviews needed (based on tags added recently).

I think that the first two tasks can be marked as complete (crossed them out), as the current state of the MR means the module is ported now. The module in the MR is very similar to the final D10 commit. One of the biggest difference is that D7 does not have a settings tray, so there is only one listing on the admin/announcements_feed. And the second difference is the placement of the Announcements link on the admin toolbar, which is different in D7 - see the image: https://git.drupalcode.org/project/drupal/uploads/62f84fcba85e5f13e6f224d5acc69f04/image.png.

Usability review

We discussed this issue at #3361526: Drupal Usability Meeting 2023-05-26. That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @shaal, and @worldlinemine.

Short version:

1. Do not use an icon.
2. By default, put the link to the left of the Help link.

Longer version:

In Drupal 10, every top-level item in the Administration menu has an icon, so it makes sense for the Announcements link to have one. It also works well when the announcements open in the settings-tray area. In Drupal 7, none of the other Toolbar items has an icon and the link opens normally in a new page.

In any list, the most prominent items are the first and the last. It is a common pattern (not just in Drupal--for example, I can glance up at the Firefox menu as I write this) to have a Help link at the end. The new link should not take that position.

We suggest giving the Announcements link weight 2, so that it appears between Configuration and Reports. The exact placement is not too important, since the site administrator can change the order of the links. If you prefer, you can give it weight 0.

Thanks @benjifisher
I have removed the icon from announcements menu and placed the menu between configuration and reports.

Are we using any of the images included in the "images" folder? If we are not, we can probably remove them.

@fjgarlin
You are right, I have removed the images folder.

I tested it all, and functionality-wise, it's all there. All the above feedback has been addressed.
I made a comment on a really minor wording issue, as the "toolbar" might not be present on all D7 sites.

It also looks really good code-wise and I'll probably mark it as RTBC once the above change has been addressed.

Thanks @fjgarlin
I have replaced the "toolbar" with "menus" as the Administration itself is structured as a menu.
This modification ensures that the statement remains applicable and coherent regardless of whether the toolbar module is enabled or disabled.

But the comment change @fjgarlin mentioned added a direct link to the feed.

Users with the "View drupal.org announcements" permission may click on the "Announcements" item in the administration toolbar, or access @link, to see all announcements relevant to the Drupal version of your site.

I think we are still missing that link?

Done

Would you mind rebasing the MR? It seems that it cannot be done via the UI.

Once it is rebased I think we can RTBC it 🙂

Sure, on it

The merge request is rebased and now it is ready the merge.

I did a final test and gave the code a final pass again.

I think all feedback from the different people that reviewed it has been addressed, and based on #54 (and the fact that I was going to RTBC after the latest text changes that I required), I am doing it now :-)

Thanks so much for all the work!!

First of all thanks for all the hard work that's gone into this so far!

I have some (mostly nitpick) comments from an initial glance at the code; I'll try to put those into the MR (but we're still using patches in general for D7 core so I'm not particularly familiar with the MR workflow yet).

One thing we'll need to consider carefully if we're going to add this to D7 core is the defaults; i.e. I wouldn't think we'll enable the module by default on existing sites? I've not reviewed how that was handled in D10 and I haven't got as far as checking whether there's anything relating to that in the current MR.

I'll carry on my initial read through and leave specific comments in the MR.

Thanks for looking into the code.

Once you are happy with the code, getting a patch version of it is really easy, you just append ".diff" or ".patch" to the URL of the MR and you can get a file in that format in the browser that you can download. For example: https://git.drupalcode.org/project/drupal/-/merge_requests/3965.patch

The D10 version of the module is not enabled by default, at least for now, whilst the module is marked as experimental.

I've added a few comments in the MR (thank you to @fjgarlin and others for helping me to do so!)

I didn't spot any significant problems and I think in general the module's looking pretty good, thanks!

For Drupal 7, the real value here is to be able to reach our community of D7 site owners before end of life, which I think is an argument in favor of enabling by default.

Given the fact how many Drupal 7 sites there are still out there I think this is a great argument. Although the question arises how many of those are actually still maintained and updated of course. But I don't see a problem with that 😉

@BramDriesen Looking at the statistics here: https://www.drupal.org/project/usage/drupal , there is a lot of D7 sites that are still maintained/updated.

All sites (reported): 391k
Drupal 7.90 (june 2022) - 7.98 (most recent): +-152k sites (one year from the last update)
Drupal 7.83 (december 2021) - 7.89 (march 2022): +-24,5k sites (one and a half year from the last update)

So it seems like that approx. 1/2 of all sites is still at least minimally maintained. More than 100k sites (1/4 of all reported sites) are on the release from last few months.

What happens if the feed doesn't successfully fetch a valid json response?

Can we have a test for that?

I have created a separate issue where we can discuss and decide whether to enable the module by default, and/or how to proceed with this (it would be better to discuss there, instead of in this long issue).

#3379442: [policy, no patch] Decide whether to enable Announcements Feed for 7.x by default

I thought that this one was already RTBC but saw it was still as Needs Review. I tried to rebase via the UI but it didn't work.

If it can be rebased, I'm happy to give it one more pass and try to RTBC it, as all feedback has been addressed so far.

@fjgarlin I have resolved all conflict and pushed, please review

I did another pass at the code and I tested the changes locally, installed and uninstalled the module, etc.
I think all the feedback has been addressed so I am going to mark this RTBC. Thanks for all the work!

Rebased the branch so the new tests are run via GitLab CI for the first time.

Added a question to the MR.

Also, one of the tests is not passing and I'm not sure why. Moving to "Needs work". I'm sure it's something small (since it's working in DrupalCI), but we need to fix it now that we're in GitLab CI.

It's not finding some of the json feeds. The routing and DNS resolution is different in GitLab CI, so we need to alter the tests slightly. I'm on the case.

Setting this one back to RTBC. The fail in GitLab CI was due to the server setup being slightly different from that of DrupalCI, so it was blocking direct access to the ".json" files.

The changes were minimal and can be seen in this compare-link: https://git.drupalcode.org/issue/drupal-3357707/-/compare/3ddee00dd28416...
The only thing we do is change the URL so that the test module outputs the file.

We can see the GitLab CI pipeline running green here: https://git.drupalcode.org/issue/drupal-3357707/-/pipelines/56103
And also DrupalCI here: https://dispatcher.drupalci.org/job/drupal_d7/284317/console

Back to RTBC.

Wow, thanks again for all the work that's gone into this.

I am pretty happy with how it's all looking, but there's one thing that's made me raise an eyebrow.

function announcements_feed_install() {
  // Creating the default values.
  variable_set('announcements_feed_max_age', 86400);
  variable_set('announcements_feed_cron_interval', 21600);
  variable_set('announcements_feed_limit', 10);
  variable_set('announcements_feed_json_url', 'https://www.drupal.org/announcements.json');
  variable_set('announcements_feed_link', 'https://www.drupal.org/about/announcements');
}

I'm not keen on the pattern of setting the defaults for variables in hook_install() and then not specifying a default in the calls to variable_get().

In D7, I'd expect to be able to delete a variable (e.g. drush vdel announcements_feed_cron_interval) to remove any customisations that have been made to it, and for that to have the effect of restoring the default.

In fact, that's a bad example because there is a default in hook_cron() for that particular variable, but there are other places where there's not, e.g.:

    $announcements_feed_json_url = variable_get('announcements_feed_json_url');
    $response = drupal_http_request($announcements_feed_json_url);

  $build = array_merge($build, array(
    '#theme' => 'announcements_feed',
    '#count' => count($announcements),
    '#feed_link' => variable_get('announcements_feed_link'),
  ));

There are examples in D7 core of calls to variable_get() that don't specify a default, but I'm not sure I can see a good reason for using that approach here?

I was asked to review as a product manager. I am not a Drupal 7 product manager, but I think it would be useful to have this in Drupal 7. There is not a specific product manager role in Drupal 7, so I'll remove the tag.

Thanks @mcdruid, that is a very good point! I agree we should correct this.

Finally I was also able to do another complex review (including the tests), so I have added comments to the MR - mostly small comments/text changes, but also a few minor code changes. As there are also required changes from #77, moving back to NW.

When making changes - please be aware that the module itself is called Announcements - this is the name displayed in the list of modules in D7 and D10 (not Announcements Feed, or Announce), only the module folder has the name "announcements_feed". Therefore the reference to "announcements feed" is a reference to the feed itself (not to a module name) and I think it should be lowercase.

We would also need to update this documentation page: https://www.drupal.org/docs/core-modules-and-themes/core-modules/announc... , as the module is still listed as Experimental and the name of the module is wrong (also the name Announce is used on a few places there). I will create an issue for this (as it is mostly related to the D10 module, not this issue).

Once all points are resolved, we will try to get a signoff from @Fabianx and I think this would be ready to go. Then the "Enable by default" implementation will be done in a separate issue once the decision here is made: #3379442: [policy, no patch] Decide whether to enable Announcements Feed for 7.x by default

Thanks all for the hard work here!

I'm guessing that part of the motivation for not specifying defaults in calls to variable_get() is to avoid repetition / duplication.

In D7 constants are quite often used for variable defaults, so that the actual default value is set in one place - for example:

modules/system/system.module:16:define('DRUPAL_CRON_DEFAULT_THRESHOLD', 10800);

modules/system/system.admin.inc:1645:    '#default_value' => variable_get('cron_safe_threshold', DRUPAL_CRON_DEFAULT_THRESHOLD),

modules/system/system.module:3577:  if (($threshold = variable_get('cron_safe_threshold', DRUPAL_CRON_DEFAULT_THRESHOLD)) > 0 && variable_get('install_task') == 'done') {

modules/update/update.module:17:define('UPDATE_DEFAULT_URL', 'https://updates.drupal.org/release-history');

modules/update/update.fetch.inc:335:  return isset($project['info']['project status url']) ? $project['info']['project status url'] : variable_get('update_fetch_url', UPDATE_DEFAULT_URL);

I agree. Let's add some constants, which are used in both places.

@mitthukumawat not sure if you're still working on this, but I have a little time today so am assigning it to myself to attend to the most recent feedback.

I believe I've addressed all of the outstanding feedback in the MR, including my own :)

Accidentally moved that to need work before. BTW I have pulled new changes and functionality is working as expected. And the latest MR feedback also resolved. RTBC +1

Thanks for addressing the remaining feedback from MR @mcdruid. I have reviewed it again and I think it looks good!

Left four additional comments in MR, but hopefully these are my last points :)

One additional question about security: Do we need to sanitize feed data (such as title, links, ...) on output somehow? I think that twig in D10 does this automatically, so this <a href="{{ announcement.url }}">{{ announcement.title }}</a> may not be the same as <a target="_blank" href="<?php print $announcement['link']; ?>"><?php print $announcement['title']; ?></a> under specific circumstances. There is a direct output of different variables in announcements_feed.tpl.php. What do you think?

I think there's one more item in the feedback to address; no idea why we've ended up with tabs in a file. I'll work out whether to fix it in vim or emacs :)

Good question on security / sanitisation. Of course we'd hope that we can trust content from this source, but we all know what assumptions lead to.

The aggregator module is hopefully a good reference; here's where it sanitises feed items:

https://git.drupalcode.org/project/drupal/blob/7.99/modules/aggregator/a...

It doesn't feel ideal to duplicate aggregator_filter_xss() but we can't rely on that module being enabled.

Do we need an allow-list like that for filtering, or would using defaults work?

I've just pushed a new template_preprocess_announcements_feed() function similar to the one in aggregator.

Also noticed that the template should have a hyphen in the name rather than an underscore.

Tests pass with this change locally.. we should add at least one test that checks the sanitisation actually works though.

@poker10 I've hopefully addressed your latest round of feedback.

Thanks! I have reviewed the last changes and everything seems good. I have also installed the module and tested it manually once again:

1. Module was installed correctly and new link was added to the menu between Configuration and Reports.
2. The link opened the feed, which was populated by 6 regular announcements as of today (which is correct, because the last one is specific for D9).
3. I have also tried removing some announcements from cache and then running cron, which added the missing announcements back.
4. Uninstallation went without problems.

All tests are green. I do not see any other unresolved feedback in the MR or in this issue. I think this is ready for commit - adding a tag for the final review.

Just noticed that we're creating the $user twice in the newest test.. I probably copy-pasted that from one of the other tests.

Should be an easy thing to tidy up before commit.

haha; wondered why a couple of $variable names didn't show up in those last commit messages.. it was exactly the same as https://www.drupal.org/project/cvslog/issues/563110 - apparently I need more coffee.

I didn't mean to set this back to NR; I've only made small tidy up changes since @poker10 RTBC'ed in #91.

Back to RTBC; I think this is ready for a final Framework Manager review.

@Fabianx has approved this (and #3424898: Enable Announcements Feed by default, but allow opt-out) in a different channel.

I think that means it's ready to commit.

Thanks to everyone who helped here! Committed and pushed the module.