Creating a legacy Redia RSS feed for events. DDFHER-60 #1584

rasben · 2024-09-18T08:42:51Z

This feed is needed for some legacy apps to continue working. It will not be updated in the future - rather, this is a 1-1 recreation of the old feed.

As I've made a custom redia legacy module, i've also moved the legacy opening hours REST api into this module.

Link to issue

https://reload.atlassian.net/browse/DDFHER-60

Old feed:

https://www.aakb.dk/ding-redia-rss/event

New feed:

https://varnish.pr-1584.dpl-cms.dplplat01.dpl.reload.dk/ding-redia-rss/event

This feed is needed for some legacy apps to continue working. It will not be updated in the future - rather, this is a 1-1 recreation of the old feed. As I've made a custom redia legacy module, i've also moved the legacy opening hours REST api into this module.

kasperg

Lots of code, lots of comments here 😅 .

I think it is a good idea to create a separate module for the legacy integrations and as I see it you have managed to recreate the feed 1-1 compared to the original.

There are a couple of places where we disagree on the overall code structure:

I would like to see the EventWrapper used more.
I would prefer more proper objects and less arrays and strings.

config/sync/core.extension.yml

web/modules/custom/dpl_redia_legacy/dpl_redia_legacy.info.yml

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

kasperg · 2024-09-20T10:05:19Z

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

+    $response = new Response();
+    $response->setContent($rss_content);
+    $response->headers->set('Content-Type', 'application/rss+xml');
+    return $response;


Please update this to return a cacheable response.

I do not know how often the feed is requested but I do not see a reason why we should not be able to cache it in Varnish.

As I see it you have changes the implementation to return a cacheable response but please also add some cacheable dependencies to make it behave right 😄

config/sync/image.style.redia_feed_large.yml

As per PR change requests, I've switched up the code to use the existing EventWrapper, using OOP rather than array loops, and generally making the RSS code more readable.

rasben · 2024-10-01T09:38:25Z

web/modules/custom/dpl_event/src/EventWrapper.php

+  /**
+   * Getting the description, from the first available text paragraph.
+   */
+  public function getDescription(): ?string {


I've moved this, from EventRestMapper, as now we use it in two places.

web/modules/custom/dpl_event/src/EventWrapper.php

rasben · 2024-10-01T10:43:42Z

@kasperg This is ready for you again. All the relevant changes have been added in the latest commit :) ("Refactor...")

web/modules/custom/dpl_update/dpl_update.install

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

kasperg · 2024-10-07T11:21:00Z

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

+      $branches = $event->get('branch')->referencedEntities();
+      $branch = reset($branches);
+      $event_dates = $event->get('date')->getValue();
+      $changed_date = DrupalDateTime::createFromFormat('U', strval($event->getChangedTime()));


As I see it the code remains. Is that intentional?

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

web/modules/custom/dpl_redia_legacy/src/RediaEventMedia.php

web/modules/custom/dpl_event/src/EventWrapper.php

kasperg · 2024-10-07T11:48:29Z

web/modules/custom/dpl_redia_legacy/src/RediaEvent.php

+/**
+ * An event object, containing the properties the RSS feed needs.
+ */
+class RediaEvent extends ControllerBase {


Please reconsider if this class and RediaEventMedia needs to extend ControllerBase.

I do not understand why this is necessary. As I see it the class is not a controller and it does not utilize any methods provided by the parent class.

They both utilize EntityTypeManager, that comes from that :)

web/modules/custom/dpl_redia_legacy/src/RediaEvent.php

kasperg · 2024-10-07T11:52:26Z

web/modules/custom/dpl_redia_legacy/src/RediaEvent.php

+  // We'll disable the documentation rules for the member properties, as
+  // they are pretty self-explanatory.
+  // phpcs:disable


Consider if is it possible to only disable the related rules for these lines instead of PHPCS entirely.

I did try, and it just refused to accept it, so this is me going nuclear.

kasperg · 2024-10-07T11:54:10Z

web/modules/custom/dpl_redia_legacy/src/Controller/RssFeeds/EventsController.php

+    $current_date = new DrupalDateTime();
+    $date = $current_date->format('r');


Getting the current time is a dependency which should be injected instead of accessed directly.

github-actions bot temporarily deployed to pr-1584 September 18, 2024 08:43 Destroyed

rasben force-pushed the DDFHER-60_redia-events branch from 0b99bf6 to a3ae90e Compare September 18, 2024 08:45

github-actions bot temporarily deployed to pr-1584 September 18, 2024 08:45 Destroyed

rasben force-pushed the DDFHER-60_redia-events branch from a3ae90e to 79168fa Compare September 18, 2024 08:51

github-actions bot temporarily deployed to pr-1584 September 18, 2024 08:51 Destroyed

rasben assigned kasperg, xendk and kasperbirch1 Sep 18, 2024

kasperg requested changes Sep 20, 2024

View reviewed changes

kasperg assigned rasben and unassigned kasperg, xendk and kasperbirch1 Sep 20, 2024

github-actions bot temporarily deployed to pr-1584 September 20, 2024 12:35 Destroyed

Re-factor Redia legacy event RSS feed. DDFFHER-60

873aa38

As per PR change requests, I've switched up the code to use the existing EventWrapper, using OOP rather than array loops, and generally making the RSS code more readable.

rasben force-pushed the DDFHER-60_redia-events branch from 37d2ad7 to 873aa38 Compare October 1, 2024 09:37

github-actions bot temporarily deployed to pr-1584 October 1, 2024 09:38 Destroyed

rasben commented Oct 1, 2024

View reviewed changes

web/modules/custom/dpl_event/src/EventWrapper.php Show resolved Hide resolved

rasben assigned kasperg and unassigned rasben Oct 1, 2024

kasperg reviewed Oct 7, 2024

View reviewed changes

rasben added 2 commits October 7, 2024 14:20

Add cachetags to Redia RSS feed. DDFHER-60

b7d5014

Merge branch 'develop' into DDFHER-60_redia-events

a61d7a6

github-actions bot temporarily deployed to pr-1584 October 7, 2024 12:21 Destroyed

Update Redia Legacy API, to match code review comments.

7cb5bc7

github-actions bot temporarily deployed to pr-1584 October 7, 2024 15:02 Destroyed

kasperg merged commit 6565b36 into develop Oct 8, 2024
19 checks passed

kasperg deleted the DDFHER-60_redia-events branch October 8, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating a legacy Redia RSS feed for events. DDFHER-60 #1584

Creating a legacy Redia RSS feed for events. DDFHER-60 #1584

rasben commented Sep 18, 2024 •

edited

Loading

kasperg left a comment

kasperg Sep 20, 2024

kasperg Oct 7, 2024

rasben Oct 1, 2024

rasben commented Oct 1, 2024

kasperg Oct 7, 2024

kasperg Oct 7, 2024

rasben Oct 7, 2024

kasperg Oct 7, 2024

rasben Oct 7, 2024

kasperg Oct 7, 2024

		$current_date = new DrupalDateTime();
		$date = $current_date->format('r');

Creating a legacy Redia RSS feed for events. DDFHER-60 #1584

Creating a legacy Redia RSS feed for events. DDFHER-60 #1584

Conversation

rasben commented Sep 18, 2024 • edited Loading

Link to issue

kasperg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasben commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasben commented Sep 18, 2024 •

edited

Loading