As far as I can see currently all comments saving is done using comment_save() function. This is nice but we have headache about validation (access as well). What I propose to do is to switch comments to drupal_form_submit like in node resource.

Please advise your comments.

CommentFileSizeAuthor
#23 1065568-d6-comment-switch-to-drupal-execute-reroll.patch14.47 KBygerasimov
#20 1065568-d6-comment-switch-to-drupal-execute.patch14.55 KBygerasimov
#11 1065568-comment-resource-tests-create-method-array.patch21.23 KBygerasimov
#9 1065568-comment-resource-tests.patch21.13 KBygerasimov
#8 1065568.patch21.12 KBgdd
#6 1065568-comment-resource-d7-tests-reroll.patch19.25 KBygerasimov
#3 1065568-comment-resource-d7-tests-review.patch19.28 KBygerasimov
#1 1065568-comment-resource-d7-tests.patch19.35 KBygerasimov

Comments

ygerasimov’s picture

ygerasimov commented
Title: Comment Resource. Move to drupal_form_submit » Comment Resource. Port to D7. Move to drupal_form_submit
Status: Active » Needs review
StatusFileSize
new 1065568-comment-resource-d7-tests.patch19.35 KB

Attached patch for porting comment resource to D7 + tests.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Needs work

Remove the watchdog, and then make sure to add back in, filter_access.

You can see my patches in #1045724: Creating comments issue for how I accomplished that,

other than that it looks good.

WEll also need to port this to 6x afterwords

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new 1065568-comment-resource-d7-tests-review.patch19.28 KB

There is no need to add filter access. You can see in tests that I try to add comment with full_html filter that is not allowed. As we run saving using form, validation error gets triggered. So we are secured that user can add comment with only allowed filter.

kylebrowning’s picture

kylebrowning commented

Right im sorry, yes, this looks good, thanks for removing the watchdog.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

This patch is actually backwards, for instance it is removing drupal_form_submit() and adding drupal_execute(). Please reroll.

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new 1065568-comment-resource-d7-tests-reroll.patch19.25 KB

rerolled patch

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented
Status: Needs review » Needs work

Setting to CNW just because the testbot is testing over and over and we need to figure out why. OK to retest later.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new 1065568.patch21.12 KB

I recently had a discussion with the security team in which they said that it was not necessary to have access to view a node in order to view its comments. Core does not check for this either. Therefore we should change our access callbacks to reflect this. I have adjusted the patch to reflect this. I don't think this affects test at all, but I'm not positive. New patch has been attached.

Otherwise I think this is good if someone else can verify.

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 1065568-comment-resource-tests.patch21.13 KB

It is strange that nodes can be viewed even when user cannot view the node. But it is ok. Tests run fine. I have removed trailing spaces.

kylebrowning’s picture

kylebrowning commented

My only concern is this,

  return (object)array(
    'cid' => $comment->cid,
    'uri' => services_resource_uri(array('comment', $comment->cid)),
  );

Why are we typecasting it to an object?
In other parts of Services (node, user) were are returning an array.

Other than that it looks great, but changing that will almost certainly affect tests so if it does chnage well need to adress that.

ygerasimov’s picture

ygerasimov commented
StatusFileSize
new 1065568-comment-resource-tests-create-method-array.patch21.23 KB

I think it is reasonable change it to array. Here is patch with all tests passing.

Meanwhile 6.x-3.x had object as answer as well.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

This one doesn't get past "queued for testing" because Services HEAD (master branch) is not passing tests: http://qa.drupal.org/pifr/test/100369

kylebrowning’s picture

kylebrowning commented

rfay, the tests pass locally. :( I think the testbot doesnt know what our dependencies are.

rfay’s picture

rfay
he,him
Primary language English
Location Palisade, CO, USA
commented

PIFR does not yet handle contrib dependencies, unfortunately.

kylebrowning’s picture

kylebrowning commented

Yeah, i had a discussion with boomba about it, but he said theres a patch that makes it work

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Reviewed & tested by the community

OK This looks good.

Now we just need the 6.x version.

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Patch (to be ported)
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
ygerasimov’s picture

ygerasimov commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1065568-d6-comment-switch-to-drupal-execute.patch14.55 KB

Here is port to D6.

kylebrowning’s picture

kylebrowning commented

#20 is failing :(

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Needs work
ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new 1065568-d6-comment-switch-to-drupal-execute-reroll.patch14.47 KB

rerolled patch attached

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Fixed

Fixed and committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.