I couldn't immediately locate the code behind this because of the use of Ctools and the fact that the "Add" menu item at /admin/structure/services doesn't tell me I'm actually adding an endpoint. The problem is that form asks you for a name and simply says it's "The unique ID for this endpoint." I entered the name "Commerce API" and then got an error message telling me I can only use alphanumeric characters and underscores. The problem is this wasn't apparent on the form at all. I'd advise at least adding this information to the form element's #description, and if possible you might consider using a machine-name field instead of a regular textfield.

CommentFileSizeAuthor
#8 services-1621512-machine-name-element-for-endpoint-name-test-8.patch5.91 KBygerasimov
#6 services-1621512-machine-name-element-for-endpoint-name-test-6.patch5.89 KBygerasimov
#1 services-1621512-machine-name-element-for-endpoint-name.patch3.27 KBygerasimov

Comments

ygerasimov’s picture

ygerasimov commented
Status: Active » Needs review
StatusFileSize
new services-1621512-machine-name-element-for-endpoint-name.patch3.27 KB

Good valid advise. Ryan do you mean something like attached patch?

Status: Needs review » Needs work

The last submitted patch, services-1621512-machine-name-element-for-endpoint-name.patch, failed testing.

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review

#1: services-1621512-machine-name-element-for-endpoint-name.patch queued for re-testing.

rszrama’s picture

rszrama commented

Ahh, yeah, I'll give it a shot. I didn't realize Ctools wasn't already using a machine-name element for that form item. Definitely the place to use it.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Needs work

This looks fine but our test coverage for these validations does not exist, can we write tests for them?

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new services-1621512-machine-name-element-for-endpoint-name-test-6.patch5.89 KB

Here is patch with test for UI.

Status: Needs review » Needs work

The last submitted patch, services-1621512-machine-name-element-for-endpoint-name-test-6.patch, failed testing.

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Needs review
StatusFileSize
new services-1621512-machine-name-element-for-endpoint-name-test-8.patch5.91 KB

Oh. Form error message has been changed. Resubmitting the patch.

kylebrowning’s picture

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

Also needs a backport!

marcingy’s picture

marcingy commented
Status: Reviewed & tested by the community » Needs work 
+  // Validate path.
+  $query = db_select('services_endpoint', 'e');
+  $query->addField('e', 'eid');
+  $query->condition('path', $form_state['values']['path']);

This does not need to be a dynamic query just use a basic db_query I realise this is existing code but lets not keep what is unnecessary. 

+function services_ctools_export_ui_form_machine_name_exists($value) {
+  // Validate Name.
+  $query = db_select('services_endpoint', 'e');
+  $query->addField('e', 'eid');
+  $query->condition('name', $value);
+
+  $result = $query->execute()->fetchField();
+  return !empty($result);
 }

Again this query does not need to be dynamic

ygerasimov’s picture

ygerasimov commented
Status: Needs work » Patch (to be ported)

I have changed patch according to marcingy comments on #10 and committed it. Commit 069324d

ygerasimov’s picture

ygerasimov commented
Status: Patch (to be ported) » Fixed

Committed to 6.x-3.x branch. Commit ca98d8e

Status: Fixed » Closed (fixed)

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