The xml for the release history and project list feeds is created manually using string concatenation. It might be better if the DOM library was used to create the feeds.

CommentFileSizeAuthor
#4 project-release-history-DOM-4.patch14.42 KBHugo Wetterberg
project-release-history-DOM.patch14.15 KBHugo Wetterberg

Comments

dww’s picture

dww
we/he/they
commented
Status: Needs review » Postponed (maintainer needs more info)

Can you explain what this buys us? Generally, I'm in favor of using APIs and objects that solve a problem for you, but it's not clear how this improves the readability or maintainability in this particular case. Generally, it makes the code less obvious (IMHO). Also, the old code would check_plain() potentially sensitive data whereas I don't see that in the new code. That seems like a regression, unless this object is doing something special I don't know. Also, what PHP version requirements does this library introduce? Can we always assume this library is available?

Thanks!
-Derek

Hugo Wetterberg’s picture

Hugo Wetterberg commented

Hi Derek,
One of the big benefits with using DOM is that it takes care of escaping everything for you, producing valid xml is kind of it's reason for existing. The DOM library *should* be available in PHP > 5 http://www.php.net/manual/en/dom.installation.php

heine’s picture

heine commented

@Hugo,

createElement doesn't handle plaintext correctly. You need to use createTextNode for these.

Hugo Wetterberg’s picture

Hugo Wetterberg commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new project-release-history-DOM-4.patch14.42 KB

Ah, thanks Heine. I actually think that I've been bitten by that behaviour before. New patch that fixed some other typos too.

dww’s picture

dww
we/he/they
commented

Sorry, I've been swamped with a lot of things (d.o redesign, Git launch, etc). ;) Just re-found this issue and realize this could be a big step in the right direction towards #1003764: Add an alter hook invoked while generating release-history XML files...

dww’s picture

dww
we/he/they
commented

Sorry, I've been swamped with a lot of things (d.o redesign, Git launch, etc). ;) Just re-found this issue and realize this could be a big step in the right direction towards #1003764: Add an alter hook invoked while generating release-history XML files...

Status: Needs review » Needs work

The last submitted patch, project-release-history-DOM-4.patch, failed testing.