Use of services to access functions

1. +++ b/google_vision.services.yml
   @@ -0,0 +1,3 @@
   +  google_vision.requests:
   

   I'm not super happy with the name of the service. Should we use google_vision.api?

2. +++ b/google_vision.services.yml
   @@ -0,0 +1,3 @@
   +    class: Drupal\google_vision\Connector
   

   Neither with the name of the class. Please create another issue for renaming that one (to the same name we decide to use in the service)

3. +++ b/src/Connector.php
   @@ -14,15 +14,13 @@ class Connector {
   -    // It looks pretty dirty. I hope that in future it will be implemented in Google SDK
   -    // and we will be able to avoid this approach.
   

   No need to remove this comment for now.

4. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    // Prepare JSON.
   

   Instead of concatening strings, let's use an array and use json_encode and json_decode.

5. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    $ch = curl_init('https://vision.googleapis.com/v1/images:annotate?key=' . $api_key);
   +    curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "POST");
   +    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
   +    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
   +    curl_setopt($ch, CURLOPT_HTTPHEADER, array(
   +        'Content-Type: application/json',
   +        'Content-Length: ' . strlen($data),
   +      )
   +    );
   

   We should use Drupal http_request service instead of curl natively.

   That service should be injected in the constructor. See https://www.drupal.org/node/2133171

   /src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    $config = \Drupal::getContainer()->get('config.factory')->getEditable('google_vision.settings');
   

   Please create a follow-up issue for injecting the configuration. Also, we shouldn't use getEditable(), that's only for config editing forms.

6. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    // Prepare JSON.
   

   Don't concatenate strings, let's use arrays and json_decode and json_encode. It's easier to read and less error prone.

7. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    $ch = curl_init('https://vision.googleapis.com/v1/images:annotate?key=' . $api_key);
   +    curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "POST");
   +    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
   +    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
   +    curl_setopt($ch, CURLOPT_HTTPHEADER, array(
   +        'Content-Type: application/json',
   +        'Content-Length: ' . strlen($data),
   +      )
   +    );
   

   Use http_client here again :)

8. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    // Prepare JSON.
   

   Another one.

9. +++ b/src/Connector.php
   @@ -59,4 +57,280 @@ class Connector {
   +    $ch = curl_init('https://vision.googleapis.com/v1/images:annotate?key=' . $api_key);
   

   Use http client.

10. +++ b/src/Connector.php
    @@ -59,4 +57,280 @@ class Connector {
    +    // Prepare JSON.
    

    Again JSON by concatening string.

11. +++ b/src/Connector.php
    @@ -59,4 +57,280 @@ class Connector {
    +    $ch = curl_init('https://vision.googleapis.com/v1/images:annotate?key=' . $api_key);
    

    Use http_client.

There are more issues that I didn't comment on, but that had the same pattern that the issues above.

Interdiff please next time https://www.drupal.org/documentation/git/interdiff

1. +++ b/google_vision.services.yml
   @@ -0,0 +1,3 @@
   +    class: Drupal\google_vision\ImageRequest
   

   Make the class name to be GooglevisionAPI.

   Pass the @config.factory as arguments here so that you can inject it in constructor of the class.

2. +++ b/src/Connector.php
   @@ -9,54 +9,39 @@ namespace Drupal\google_vision;
    
   

   Remove extra lines b/w use statements.

3. +++ b/src/Connector.php
   @@ -9,54 +9,39 @@ namespace Drupal\google_vision;
      /**
   

   add a new line after protected property.

4. +++ b/src/ImageRequest.php
   @@ -0,0 +1,62 @@
   +    $ch = curl_init('https://vision.googleapis.com/v1/images:annotate?key=' . $api_key);
   +    curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "POST");
   +    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
   +    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
   +    curl_setopt($ch, CURLOPT_HTTPHEADER, array(
   +        'Content-Type: application/json',
   +        'Content-Length: ' . strlen($data),
   +      )
   +    );
   

   Use drupal httpClient over curl

@ajalan065 Your patch doesn't apply to 8.x-1.x. Did you built this patch in top of other patches?

1. +++ b/src/Connector.php
   @@ -8,54 +8,91 @@
   +   */
   

   document the parameter in function. using @param https://www.drupal.org/node/1354#param

2. +++ b/src/Connector.php
   @@ -8,54 +8,91 @@
   +  public function labelDetection($filepath) {
   

   same here document the parameter and document the return value.

3. +++ b/src/Connector.php
   @@ -8,54 +8,91 @@
   +  public function landmarkDetection($filepath) {
   

   same as above.

4. +++ b/src/Connector.php
   @@ -8,54 +8,91 @@
   +    $req = $this->imageRequest->makeRequestForLandmark($filepath);
   

   $request is better name.same at other places as well.

5. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,327 @@
   +/**
   + * @file
   + * Google vision connector.
   + */
   

   Remove @file

6. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,327 @@
   +    $config = $this->configFactory->getEditable('google_vision.settings');
   

   Why editable config ? why not get b/c you are not updating/saving the configuration later.

7. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,327 @@
   +    return json_decode($response->getBody(), TRUE);
   

   Use Json utility class and use its static function.

8. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,327 @@
   +    return json_decode($response->getBody(), TRUE);
   

   same as above. Use Json utility class.

1. +++ b/src/Connector.php
   @@ -1,61 +1,133 @@
    /**
   - * @file
     * Google vision connector.
     */
   

   Remove this whole with comments as well b/c we don't need this basic description.

2. +++ b/src/Connector.php
   @@ -1,61 +1,133 @@
   +   * @var \Drupal\google_vision
   

   This should be @var \Drupal\google_vision\GoogleVisionAPI

3. +++ b/src/Connector.php
   @@ -1,61 +1,133 @@
   +  protected $request;
   

   better name of this property is $googleVisionAPI

   I believe

4. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,354 @@
   +/**
   + * Google vision connector.
   + */
   

   Remove this description.

5. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,354 @@
   +   * Constructor.
   

   This line should be "Construct an GoogleVisionAPI object."

6. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,354 @@
   +   * @param file URI $filepath.
   

   file URI is not return type so it should be "@param string $filepath"

7. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,354 @@
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   ...
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   ...
   +    $config = $this->configFactory->get('google_vision.settings');
   +    $api_key = $config->get('api_key');
   +    if (!$api_key) {
   +      return FALSE;
   +    }
   

   We have this functionality common in every function. So add a new property to the class named $apiKey.

   In constructor set the value of $apiKey after getting it from configFactory and only do the check in the function.
   Also add a @todo we will throw exception later when we will add about the api key not set on status page.
   Create a new issue for i.

8. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,354 @@
   +    $client = \Drupal::httpClient();
   

   Now there's lot of cleanup has done. so its time to inject the httpClient service as well.

nice improvement. its about to go, only few nitpicks needed.

1. +++ b/src/Connector.php
   @@ -1,61 +1,129 @@
   +   * @param \Drupal\google_vision\GoogleVisionAPI $google_VisionAPI.
   

   The variable name can be more properly named as $google_vision

2. +++ b/src/Connector.php
   @@ -1,61 +1,129 @@
   +
   

   extra line.not needed.

3. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,350 @@
   +   * @return Array.
   

   the return will be "@return Array|bool" Update at other places as well.

4. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,350 @@
   +    $url = 'https://vision.googleapis.com/v1/images:annotate?key=' . $this->apiKey;
   

   instead of keeping the url hard coded. Move it to a constant in the class.

   Add a new const
   const APIEndpoint = 'https://vision.googleapis.com/v1/images:annotate?key='; and use constant in place of hard coded value.

5. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,350 @@
   +    return Json::decode($response->getBody(), TRUE);
   

   Where are we handing the errors in response ? Create a new issue for handing errors from Vision api.

Now looks good to be in.

1. +++ b/src/Connector.php
   @@ -1,61 +1,120 @@
   +class Connector extends ControllerBase {
   

   Why do we need to extend ControllerBase if this is not a controller?

   Also, we need phpdoc documenting this class.

2. +++ b/src/Connector.php
   @@ -1,61 +1,120 @@
   +  public function labelDetection($filepath) {
   +    $req = $this->googleVisionAPI->makeRequestForLabels($filepath);
   +    return $req;
   +  }
   

   Why do we need this service, if we forward all method calls to the API directly? Wondering if we only need one service.

3. +++ b/src/GoogleVisionAPI.php
   @@ -0,0 +1,355 @@
   +    return Json::decode($response->getBody(), TRUE);
   

   Let's handle errors here.

Let's do that, and let's update the issue summary. We should add tests ASAP, but I won't block this one on that.

I'm happy with this at this stage, we can improve it further in new issues. Can we update the issue summary and title for having an updated description of what we've done here?

This looks RTBC for me.

Hi all

I have reviewed the patch. Good job but I think that you have a lot of duplicated strings. It's great that we have separate method for each kind of request, but inside of each method we have almost the same code. Would be nice to have few private methods to call them inside of method for each kind of request and avoid duplicating.
What do you think?

I agree, but wanted to move the ball forward and not having an infinite issue. Actually, I'm wondering if we should use https://github.com/ThijsFeryn/google-cloud-vision-api instead, that's why I didn't bother that much about the code duplication.

Actually, I'm wondering if we should use https://github.com/ThijsFeryn/google-cloud-vision-api instead, that's why I didn't bother that much about the code duplication.

Why not, but maybe in future? Now this repository looks pretty young. It can be changed unexpectedly. Let's wait a little.
@ajalan065 could you create separate issue for this idea to do not loose it?

I agree, but wanted to move the ball forward and not having an infinite issue.

Yes, I agree.

I have trouble with module file entity. Give me a little time to resolve it and test the patch.

Fatal error: Class 'Drupal\google_vision\RequestOptions' not found in /home/eugene/prj/home/test8/modules/contrib/google_vision/src/GoogleVisionAPI.php on line 67

Where can I obtain this class?

You definitely need a usage statement: use GuzzleHttp\RequestOptions;.

+++ b/src/GoogleVisionAPI.php
@@ -53,6 +53,24 @@ class GoogleVisionAPI {
+    $url = APIEndpoint . $this->apiKey;

Where does APIEndpoint come from?

So I'm sure (and sad) that you didn't test this.

Let's explicitly use static::APIEndpoint then.

Great! Committed.

Yes, I just want to see that commit is displayed on site.

I see them in repository. So let's close issue.

@eugene.ilyin,

have you pushed to the d.o. git repo ?

@ajalan065
the issue status should be fixed. read about issue statuses https://www.drupal.org/issue-queue/status