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

Fix/remove custom xmlrss #14034

Merged
merged 6 commits into from
Aug 14, 2018
Merged

Conversation

alroniks
Copy link
Collaborator

@alroniks alroniks commented Aug 9, 2018

What does it do?

It removes custom classes for parsing RSS feeds in dashboard widgets by replacing them SimplePie library (composer depps).

Why is it needed?

Because an original refactoring pull request is too big to review it will help to bring fixes a bit faster splitting refactoring into small pieces.

Related issue(s)/PR(s)

#13900

composer.json Outdated
"classmap": ["core/xpdo/"]
"classmap": [
"core/xpdo/",
"core/model/modx"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this necessary here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've experimented with external core and found out that MODX classes not added to autoload configuration of the composer. So maybe this PR not the best place for this change, but it should be in MODX 3 to get MODX core classes available via autoload.php.

@Mark-H
Copy link
Collaborator

Mark-H commented Aug 9, 2018

schermafbeelding 2018-08-09 om 17 10 22

❤️

}
return $this->success('', array('html' => implode("\n",$o)));

return $this->success('', array('html' => implode(PHP_EOL, $output)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the short array syntax [] here as well?

Mark-H
Mark-H previously approved these changes Aug 9, 2018
Copy link
Collaborator

@Mark-H Mark-H 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, thanks @alroniks. Suggestion from @JoshuaLuckers is valid and would be welcome but I'd be happy to see this merged without as well. After 2.7 when 2.x has been merged into 3.x I'd love to throw phpcs and phpcbf against the entire codebase to get all different code styles cleaned up. :D

@alroniks
Copy link
Collaborator Author

@Mark-H I've removed also mentioning of xmlrss from the build configuration and replaced array by short syntax.

@alroniks alroniks added the pr/review-needed Pull request requires review and testing. label Aug 12, 2018
@alroniks
Copy link
Collaborator Author

Hi @opengeek Could we delete this files? Yes, I am asking for approve :)

Copy link
Member

@opengeek opengeek left a comment

Choose a reason for hiding this comment

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

Please don't just change syntax (array syntax in this case) or formatting when making specific other changes like this in the future. We can apply those is atomic commits to avoid unnecessary conflicts.

@Mark-H Mark-H added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Aug 13, 2018
@ilyautkin
Copy link
Contributor

PR is tested. Feed widgets works after composer update. No bugs found.

@alroniks alroniks merged commit efcb5b6 into modxcms:3.x Aug 14, 2018
alroniks added a commit that referenced this pull request Aug 14, 2018
…ibrary [#14034]

* upstream/pr/14034:
  Array short syntax
  Removed xmlrss from build configuration
  Removed modx from classmap to avoid double required classes
  Using SimplePie directly in the widget
  Added by composer class SimplePie for parsing rss feeds
  Removed outdated code
@alroniks alroniks deleted the fix/remove_custom_xmlrss branch August 14, 2018 07:25
@opengeek opengeek added this to the v3.0.0-alpha1 milestone Dec 10, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants