Opened 4 months ago

Closed 3 months ago

#16639 closed Task (fixed)

Make callback parameter in ajax.post optional

Reported by: m.lewandowski Owned by: m.lewandowski
Priority: Normal Milestone: CKEditor 4.6.1
Component: General Version:
Keywords: Cc:

Description

While building JsDuck produces following warning:

Warning: /path/to/dev/ckeditor-dev/plugins/ajax/plugin.js:137: Optional param followed by regular param callback

We need to make callback parameter also optional, it's enough to modify plugin's internal post method, and just conditionally call callback.

Change History (8)

comment:1 Changed 4 months ago by m.lewandowski

  • Status changed from new to confirmed

comment:2 Changed 4 months ago by m.lewandowski

  • Summary changed from Fix ajax.post docs to Make callback parameter in ajax.post optional

comment:3 Changed 3 months ago by m.lewandowski

  • Owner set to m.lewandowski
  • Status changed from confirmed to assigned

comment:4 Changed 3 months ago by m.lewandowski

  • Status changed from assigned to review

Pushed to branch:t/16639.

comment:5 Changed 3 months ago by k.krzton

  • Status changed from review to review_failed

The solution is fine. However, there is one, very small issue. The added manual test says:

Expected: No JavaScript exceptions are logged into the console.

There is one exception logged every time you click send POST request:

plugin.js:112 POST http://tests.ckeditor.dev:1030/tests/plugins/ajax/manual/post404.html 404 (Not Found)

I know it is not connected with the fix and probably was done on purpose (post404.html suggests it), but I think it might be quite confusing in this particular case.

comment:6 Changed 3 months ago by m.lewandowski

  • Status changed from review_failed to review

I've tried to use existing resource for POST requests, but Bender doesn't handle POST at all - thus changing Bender behavior for this little ticket will be too time consuming. But I have updated the manual test description to mention that 404 error will be reported.

Pushed to the same branch.

comment:7 Changed 3 months ago by k.krzton

  • Status changed from review to review_passed

LGTM!

comment:8 Changed 3 months ago by k.krzton

  • Resolution set to fixed
  • Status changed from review_passed to closed

Merged to master with 79201bd.

Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy