Skip to content

[4.2] Remove mocking XMLReader as it's bad practice. Fix parsing version#38095

Merged
HLeithner merged 2 commits intojoomla:4.2-devfrom
wilsonge:xml_reader_tests
Jun 24, 2022
Merged

[4.2] Remove mocking XMLReader as it's bad practice. Fix parsing version#38095
HLeithner merged 2 commits intojoomla:4.2-devfrom
wilsonge:xml_reader_tests

Conversation

@wilsonge
Copy link
Copy Markdown
Contributor

@wilsonge wilsonge commented Jun 19, 2022 •
edited
Loading

Summary of Changes

Fixes tests to run on PHP 8.1. Generally we shouldn't be mocking core PHP classes so this fixes:

  • Everywhere we mocked and then didn't even mock any values/return types etc. Just insert a real dummy instance
  • The one place we did test was mocking versions. Here however this covered up a bug because when turning to concrete tests it turned out we parsed the XML version in the XML doc type rather than the version of the feed being used :D This bug is fixed as part of this and one extra test case in atom added

Testing Instructions

Check Drone 8.1 tests

Actual result BEFORE applying this Pull Request

All tests related to feed do not pass

Expected result AFTER applying this Pull Request

All tests related to feed do pass (note session tests do fail - but that will be the subject of a different PR)

Documentation Changes Required

None

@wilsonge wilsonge requested review from HLeithner and Hackwar June 19, 2022 10:57
rdeutz
rdeutz approved these changes Jun 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rdeutz rdeutz left a comment

Choose a reason for hiding this comment

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

looks good to me

HLeithner
HLeithner approved these changes Jun 19, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

Looks good to me but I think needs real tests for our RSS feed.

@wilsonge
Copy link
Copy Markdown
Contributor Author

wilsonge commented Jun 19, 2022

Agreed but probably outside the scope of this PR

@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Jun 19, 2022

I mean a really test in an installed cms to merge this PR ;) not only codereview

@wilsonge
Copy link
Copy Markdown
Contributor Author

wilsonge commented Jun 19, 2022 •
edited
Loading

Screenshot 2022-06-19 at 21 35 22

Screenshot 2022-06-19 at 21 35 17

Took a 3rd party one in too for good measure

@wilsonge wilsonge changed the title Remove mocking XMLReader as it's bad practice. Fix parsing version [4.2] Remove mocking XMLReader as it's bad practice. Fix parsing version Jun 19, 2022
@laoneo laoneo added the Maintainers Checked Used if the PR is conceptional useful label Jun 20, 2022
@HLeithner HLeithner merged commit dbb77cd into joomla:4.2-dev Jun 24, 2022
@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Jun 24, 2022

Thanks

@wilsonge wilsonge deleted the xml_reader_tests branch June 24, 2022 10:02
@wilsonge
Copy link
Copy Markdown
Contributor Author

wilsonge commented Jun 24, 2022

Thankyou!

@wilsonge wilsonge added this to the Joomla 4.2.0 milestone Jun 24, 2022
This was referenced Jul 20, 2022
Closed
Merged
@rdeutz rdeutz mentioned this pull request Aug 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hackwar Hackwar Hackwar approved these changes

@rdeutz rdeutz rdeutz approved these changes

@HLeithner HLeithner HLeithner approved these changes

Assignees

No one assigned

Labels

Maintainers Checked Used if the PR is conceptional useful Unit/System Tests

Projects

None yet

Milestone

Joomla! 4.2.0

Development

Successfully merging this pull request may close these issues.

7 participants

@wilsonge @HLeithner @Hackwar @rdeutz @laoneo @joomla-cms-bot @fancyFranci