The StartsWith, Contains or EndsWith operators do not work from the XeroQuery service injected into my controller. Using these operators will return no results. The == and != operators do work.

$contacts = $this->xeroQuery
      ->setType('xero_contact')
      ->addCondition('EmailAddress', 'xero1', 'StartsWith')
      ->execute();

Returns no results

$contacts = $this->xeroQuery
      ->setType('xero_contact')
      ->addCondition('EmailAddress', 'xero1@opuslocus.com', '==')
      ->execute();

Returns a contact record (as does omitting the operator).

CommentFileSizeAuthor
#10 xero-non-boolean-multiple-conditions-12731477-10.patch4.46 KBmradcliffe
#10 interdiff-2993215-9-10.txt1.19 KBmradcliffe
#9 xero-non-boolean-multiple-conditions-12731477-9.patch3.17 KBjohnpitcairn
#4 xero-non-boolean-multiple-conditions-12731477-4.patch2.9 KBjohnpitcairn

Comments

John Pitcairn created an issue. See original summary.

johnpitcairn’s picture

johnpitcairn commented

The response is 400 Bad Request. Looks like any non-boolean condition on optional Xero fields must be preceded by a null guard condition.

FirstName!=null&&FirstName.Contains("J")

I don't think there is currently any way to add such a condition via XeroQuery and obtain the correct URL format, is there? We want null without quotes.

johnpitcairn’s picture

johnpitcairn commented
Title: Non-boolean query operators not working » Non-boolean query operators and multiple conditions not working
johnpitcairn’s picture

johnpitcairn commented
Status: Active » Needs review
StatusFileSize
new xero-non-boolean-multiple-conditions-12731477-4.patch2.9 KB

After adding NOT NULL as an operator and converting that to [fieldname]!=null, the resulting query still doesn't work because query conditions are currently concatenated by a space. This patch:

  • Provides NOT NULL as a query condition operator
  • Concatenates query conditions using AND
  • Updates Readme.md to document the above, including an example of a null field guard

Getting OR groups working is perhaps a followup issue?

johnpitcairn’s picture

johnpitcairn commented
Issue summary: View changes
johnpitcairn’s picture

johnpitcairn commented
Title: Non-boolean query operators and multiple conditions not working » Multiple conditions or non-boolean conditions on optional fields not working
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work

Thank you for the patch.

I went back to look at the old PHP-Xero library, and it is pretty much doing this so this is a regression from 7.x-1.x. It also only supported &&.

+      $value = implode(' AND ', $this->conditions);
  1. I think this will error out if there is only one condition.
    1. Maybe instead of implode, loop and check if the previous condition has an operator, if it doesn't, add AND by default. That would probably improve developer experience and improving test coverage.
    2. Validate could make sure each condition has an operator after it if >1 and doesn't begin or end with an operator. Improving the test coverage would be nice for this too.

2. XeroQuery has the addOperator method, but no real examples around it, which basically just tacks on AND (or OR). Do you think this is a bit too cumbersome as an api user?

   $query
      // Adding the not null.
      ->addCondition('FirstName', '', 'NOT NULL')
      ->addOperator('AND')
      ->addCondition('FirstName, 'John', '==');

This seems to work fine if we add the NOT NULL portion.

(Edit: I think that doing 1.1 above and 3 below would be much easier to use).

3. It might be possible to take a look at the definition and auto-add the not null, but that could break existing uses so probably would need to add an optional parameter onto addCondition (or duplicate the method). Maybe checkNull = FALSE. What do you think?

I think that complex condition groups would be nice, but yes, agreed that should be a follow-up.

johnpitcairn’s picture

johnpitcairn commented

Ha, I totally missed the addOperator method. Yes, imploding with AND does error if there is only one condition.

Personally I don't see a need to auto-concatenate conditions using AND if operators have not been explicitly added. Let's just document the addOperator method.

Since Xero supports both ==null and !=null conditions, I think it might be reasonable to explicitly support both those as NULL and NOT NULL operations? I can see a case for wanting to find all contacts where the email field is empty, for example.

I also think it's fine for the developer to be responsible for adding the null field guard condition, let's make that possible and documented (which has the benefit of also providing an addOperator example).

Then the resulting query construction code is very clear and doesn't depend on any magic behavior in XeroQuery.

johnpitcairn’s picture

johnpitcairn commented
Status: Needs work » Needs review
StatusFileSize
new xero-non-boolean-multiple-conditions-12731477-9.patch3.17 KB

This patch:

Adds NULL and NOT NULL operations, mapped to ==null and !=null in the query string.

Updates README.md to document the addOperator() method, comparison operations, and provide examples with comparison operations, multiple conditions and a null field guard.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
StatusFileSize
new interdiff-2993215-9-10.txt1.19 KB
new xero-non-boolean-multiple-conditions-12731477-10.patch4.46 KB

This looks good.

I expanded the unit test coverage for the new conditions.

johnpitcairn’s picture

johnpitcairn commented
Related issues: +#2993692: Documentation and examples are outdated
johnpitcairn’s picture

johnpitcairn commented

Looks good to me. Mostly my patch, so I shouldn't RTBC?

  • mradcliffe committed f74f434 on 8.x-1.x 
    Issue #2993215 by John Pitcairn, mradcliffe: Adds null/not null operator...
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Fixed

Okay, I think that's fine. Thank you.

Committed.

Status: Fixed » Closed (fixed)

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