Closed dshanske closed 6 years ago
thanks for the work here! these are all great fixes.
if the replace fix depends on replace now happening after delete, please add a comment there saying so, and why. also, it sounds like we need at least a couple new tests, for the micropub_post_content filter and replace vs delete, if not others too.
finally, it looks like there are as many as four or more unrelated changes in this PR? PHP notices, media endpoint query, media endpoint permissions, post_content filter and replace vs delete. we've talked before about how closely related changes in the same PR is ok, but unrelated changes should generally be in separate PRs. please do that in the future.
thanks again!
@snarfed These are all related to the PHP Notices reported in #150, that's why I bracketed them in one single PR. But you are right. All reported together but technically separate issues. Will document and add the appropriate tests, as there was no test that will confirm the filter is working as expected...namely as a filter.
@snarfed I made some changes in the render testing, but also moved summary so it is part of the render class, which makes sense, as taking summary and setting it as the content is a rendering function, and shouldn't be in the endpoint itself.
This fixes the errors noted in #150 removing the remaining PHP notices as well as fixing the error of the media endpoint having no query option.
The micropub_post_content filter was completely ignoring that $post_content is passed through and replacing it with the content from properties, which defeats the purpose of a filter, so it now passes the content through to be altered.
However, this revealed a problem that replace occurred before delete, and therefore, it was overriding replace and setting things to null, so I swapped the two. In theory, if you are deleting and replacing something in the same transaction, which test_update does, you should delete first anyway.