At present, the module supports the Label Detection for a given image. The function establishing a connection with the API resides in a class Connector, which is being statically called inside the module.
Also, the present function uses curl.

Task:
1. Create service for the connecting functions, and use of containers.
2. Use of Drupal httpClient over curl.
3. Add functions for the remaining functionalities which the API supports, including Landmark Detection, Logo Detection, Text Detection, Safe Search Detection, Face Detection and Image Attributes.

(This is the first task of my GSOC project)

The last patch posted in the issue implements the following points:
1. Services have been created and implemented instead of static calls to the functions in the module.
2. curl has been replaced by httpClient service.
3. Functions for the remaining functionalities supported by the API have been added to the class.
4. Instead of concatenating strings in the data (which is to be sent to the API), we now use arrays.
5. Implementation of Json utility class and its static functions.
6. Service Connector has been replaced by service GoogleVisionAPI.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

ajalan065 created an issue. See original summary.

ajalan065’s picture

Category: Feature request » Task
Status: Active » Needs review
FileSize
8.99 KB

Here's the patch of the progress.

penyaskito’s picture

Status: Needs review » Needs work
  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.

ajalan065’s picture

As mentioned in the first point above, let us keep the name of the service as google_vision.api
And also renaming the class as Api.
I will create another issue for renaming the class name.

ajalan065’s picture

naveenvalecha’s picture

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’s picture

FileSize
5.93 KB

The patch with services used through container.
And also tried using httpClient() but error showing up

ajalan065’s picture

Sorry for the extra lines and spaces above.
That was kept for clear view. I will remove them in subsequent patches.

penyaskito’s picture

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

ajalan065’s picture

I built the patch on two commits. May be that is the reason. I am posting a new one.

ajalan065’s picture

ajalan065’s picture

As the past patches were not working, meaning there was something on my local which has not been reflected, so I have created a new patch entirely from scratch. So please reroll the module to its initial commit, and then apply the patch which I am uploading again.

ajalan065’s picture

Status: Needs work » Needs review
FileSize
13.77 KB
6.09 KB

Attached is the complete patch with the functions moved to services. And also the suggestions in #3 and #6 has been implemented.

An interdiff of the further progress of the work from the last patch in #12 is also attached.

Kindly review the work.

naveenvalecha’s picture

Status: Needs review » Needs work
  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.

ajalan065’s picture

Status: Needs work » Needs review
FileSize
14.82 KB
8.12 KB

The changes have been made as per the comments in #14.
Here is the new patch along with the interdiff.

naveenvalecha’s picture

Status: Needs review » Needs work
  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.

ajalan065’s picture

Status: Needs work » Needs review
FileSize
14.58 KB
11.67 KB

As per the suggestions in #16, here is the new patch along with the interdiff.

naveenvalecha’s picture

Status: Needs review » Needs work

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.

ajalan065’s picture

Status: Needs work » Needs review
FileSize
14.43 KB
6.87 KB

All the points as mentioned in #18 has been implemented in the new patch, which is uploaded here along with the interdiff.

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Now looks good to be in.

ajalan065’s picture

Thanks :)

penyaskito’s picture

Status: Reviewed & tested by the community » Needs work
  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.

ajalan065’s picture

As in 2nd point, I'll remove the class Connector and keep only one service GoogleVisionAPI.

penyaskito’s picture

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.

ajalan065’s picture

Here is the patch and the interdiff, implementing the suggestions of #22.

penyaskito’s picture

Status: Needs review » Needs work

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?

ajalan065’s picture

Title: Moving the common functions to a service » Use of services to access functions
Issue summary: View changes
Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

This looks RTBC for me.

eugene.ilyin’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
72.87 KB

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?

diff

penyaskito’s picture

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.

ajalan065’s picture

Status: Needs work » Needs review
FileSize
11.16 KB
3.8 KB

Here is the patch as per #29.

eugene.ilyin’s picture

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.

eugene.ilyin’s picture

Status: Needs review » Needs work

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?

ajalan065’s picture

Strange!!
The code runs for me without any such error.

penyaskito’s picture

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.

ajalan065’s picture

APIEndpoint is a constant which I had defined in the file.
const APIEndpoint = 'https://vision.googleapis.com/v1/images:annotate?key=';

And I had tested it. But was not receiving any such error.

penyaskito’s picture

Let's explicitly use static::APIEndpoint then.

ajalan065’s picture

Ok. I'll post the new patch implementing both of your points. :)

ajalan065’s picture

Status: Needs work » Needs review
FileSize
11.2 KB
802 bytes

Here is the new patch as per #35 and #37.

eugene.ilyin’s picture

Great! Committed.

ajalan065’s picture

So, should this issue be marked as Closed(fixed)?

eugene.ilyin’s picture

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

eugene.ilyin’s picture

Status: Needs review » Fixed

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

naveenvalecha’s picture

@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

ajalan065’s picture

I am sorry. I actually meant it as fixed. Just slip of mind to write it as closed :). I understand that it would automatically be marked as closed after two weeks of inactivity.

naveenvalecha’s picture

Assigned: ajalan065 » Unassigned

  • naveenvalecha committed 808ac70 on 8.x-1.x
    Revert "Issue #2731801 by ajalan065, eugene.ilyin, penyaskito,...

Status: Fixed » Closed (fixed)

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