Skip to content

fixed search with 0#7

Merged
chfabbro merged 1 commit intoadobe:masterfrom
dmytrokaplin:search_with_0
Oct 1, 2019
Merged

fixed search with 0#7
chfabbro merged 1 commit intoadobe:masterfrom
dmytrokaplin:search_with_0

Conversation

@dmytrokaplin
Copy link
Copy Markdown
Contributor

If I want to search by 0 I get errors "Should not be blank or null values in kewywords field". I fixed it.

@chfabbro
Copy link
Copy Markdown
Member

Hi @jeysmook. I appreciate the PR. We are doing some internal reorganization of the SDKs and who manages them, so it may take a little time before it can be merged.

@sivaschenko
Copy link
Copy Markdown
Member

sivaschenko commented Sep 17, 2019

The issue description:

Steps to reproduce:
Use SDK to search for "0". (SearchParameters::setWords("0"))

Actual Result:
StockApiException 'Should not be blank or null values in kewywords field' is thrown

Expected Result:
"0" words search parameter is set successfully, as a result, it is possible to retrieve relevant search results for query "0"

Additional Info:

When SearchParameters::setWords is called with argument value "0" the condition empty("0") is resolved as "true" and the exception is thrown.

Current pull request is correcting the condition to explicitly check for empty string. "0" === "" is false

Also, as the SearchParameters::setWords function declaration has strict typed parameter string the function will not accept any parameter value that is not a string. (TypeError is thrown on an attempt to pass a value that is not string)

Copy link
Copy Markdown
Member

@chfabbro chfabbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivaschenko and @jeysmook: I discussed this with the Stock team, and they made the following recommendations:

Since it's valid to make a search request with no keywords and we want to allow that for the SDK users then I think you should remove it.
Just make sure that where the value is used down the line to create the stock api request is valid and does not do something like search_parameters%5bwords%5d=null

Net, we recommend removing lines 516-517, if you can verify that only a string value will be passed to the API. BTW, null and undefined are valid search terms :) so perhaps the string type check in the method parameters is enough? Can you also please validate that text is properly escaped before sending to the API, so no one can pass control/escape codes to the server? Thanks!

public function setWords(string $words)

Comment thread src/Models/SearchParameters.php Outdated
@dmytrokaplin
Copy link
Copy Markdown
Contributor Author

@chfabbro I added an update. Please see it.

Copy link
Copy Markdown
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you @jeysmook !

The file is reformatted according to best practices - unnecessary spaces removed from empty lines ✔️

Comment thread src/Models/SearchParameters.php
@chfabbro
Copy link
Copy Markdown
Member

chfabbro commented Oct 1, 2019

I can't accept it in this state. Somebody's IDE changed the formatting of every line to add additional space. Stock standards don't allow formatting changes without a separate ticket.

Please don't change the formatting when submitting PRs.

@dmytrokaplin
Copy link
Copy Markdown
Contributor Author

@chfabbro Hi! I modified updates. Please see it.

@chfabbro chfabbro merged commit 0801a4a into adobe:master Oct 1, 2019
@chfabbro
Copy link
Copy Markdown
Member

chfabbro commented Oct 1, 2019

Looks good. Thank you for reverting the format changes!

sivaschenko added a commit to sivaschenko/stock-api-libphp that referenced this pull request Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants