Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redia XML fixes #1725

Merged
merged 10 commits into from
Dec 2, 2024
Merged

Redia XML fixes #1725

merged 10 commits into from
Dec 2, 2024

Conversation

kasperg
Copy link
Contributor

@kasperg kasperg commented Nov 5, 2024

Link to issue

https://reload.atlassian.net/browse/DDFHER-117
https://reload.atlassian.net/browse/DDFHER-124
https://reload.atlassian.net/browse/DDFHER-125

Description

The current approach is somewhat frail in regards to ensuring
correct and consistent formatting.

In practice it also has challenges given the fact that it requires us
to ensure that our values are properly encoded. We have currently done
this for the description but in practice we also need to do this for
all other values.

To avoid this we update the implementation to use core PHP XML Writer
instead.

It has a number of advantages over alternate solutions:

  • SimpleXML does not allow the same amount of control of the output.
    Example; I could not get it to add an explicit encoding attribute on
    the root XML element.
  • Third party solutions add to our dependency tree. We do not want to
    do this to maintain a legacy solution.

The new implementation has been checked by matching it with the
existing one using semantic diff'ing with difftastic.

It matches 1-to-1 except for the following exception: When data is
missing e.g. branch or media then the related elements are omitted
instead of returning empty elements.

By doing this we can drop the encoding of the description we already
added.

Additionally more fields from the original feed have been added.

Additional comments or questions

I recommended reviewing this commit by commit.

The current approach is somewhat frail in regards to ensuring
correct and consistent formatting.

In practice it also has challenges given the fact that it requires us
to ensure that our values are properly encoded. We have currently done
this for the description but in practice we also need to do this for
all other values.

To avoid this we update the implementation to use core PHP XML Writer
instead.

It has a number of advantages over alternate solutions:

- SimpleXML does not allow the same amount of control of the output.
Example; I could not get it to add an explicit encoding attribute on
the root XML element.
- Third party solutions add to our dependency tree. We do not want to
do this to maintain a legacy solution.

The new implementation has been checked by matching it with the
existing one using semantic diff'ing with difftastic.

It matches 1-to-1 except for the following exception: When data is
missing e.g. branch or media then the related elements are omitted
instead of returning empty elements.

By doing this we can drop the encoding of the description we already
added.
It should be medium - not contentmedium.
Sometimes we want to display ranges of prices in situations where it
is not appropriate to apply the additional formatting that we normally
do (current prefix, "free" instead of 0 etc.)

One example for this is the RSS feed for the Redia app.

This adds support for this supported by a unit test.
Prices in the original feed are (mostly) displayed as raw integers in
the format "[price]" or "[low price] - [high price]". If the event is
free then there is no price element for it in the feed.

Add support for similar output in the our new implementation of the
feed by using the raw price range formatting.
Copy link
Contributor

@rasben rasben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through, commit by commit, and i dont have any comments in regards to the code.

I havent checked the actual feed, as I know it'll be tested by Redia

@rasben rasben assigned kasperg and unassigned xendk, rasben and kasperbirch1 Nov 8, 2024
@kasperg kasperg added the jira-testable ready for DDF test, and moved in Jira label Nov 22, 2024
# Conflicts:
#	web/modules/custom/dpl_event/src/EventWrapper.php
@kasperg kasperg merged commit 3476668 into develop Dec 2, 2024
19 checks passed
@kasperg kasperg deleted the redia-xml-fixes branch December 2, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-testable ready for DDF test, and moved in Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants