Code refactoringconcurrency

Function yourls_add_new_link() was an atrocious piece of code with unnecessary vars, duplicated code and convoluted logic. Now much less atrocious.

2 files changed, 103 insertions, 108 deletions

diff --git a/includes/functions-shorturls.php b/includes/functions-shorturls.php
index bacb7395..50e6dc1d 100644
--- a/includes/functions-shorturls.php
+++ b/includes/functions-shorturls.php
@@ -5,6 +5,7 @@
  */
 
 
+
 /**
  * Add a new link in the DB, either with custom keyword, or find one
  *
@@ -12,14 +13,17 @@
  *    status: string, 'success' or 'fail'
  *    message: string, a descriptive localized message of what happened in any case
  *    code: string, a short descriptivish and untranslated message describing what happened
- *
- * Depending on the operation, it will contain any of the following keys:
  *    errorCode: string, a HTTP status code
- *    url: array, the short URL creation information, with the following keys: 'keyword', 'url', 'title', 'date', 'ip'
+ *    statusCode: string, a HTTP status code
+ * Depending on the operation, it will contain any of the following keys:
+ *    url: array, the short URL creation information, with keys: 'keyword', 'url', 'title', 'date', 'ip', 'clicks'
  *    title: string, the URL title
  *    shorturl: string, the proper short URL in full (eg 'http://sho.rt/abc')
  *    html: string, the HTML part used by the ajax to update the page display if any
  *
+ * For compatibility with early consumers and third parties, when people asked for various data and data formats
+ * before the internal API was really structured, the return array now collects several redundant information.
+ *
  * @param  string $url      URL to shorten
  * @param  string $keyword  optional "keyword"
  * @param  string $title    option title
@@ -28,17 +32,30 @@
 function yourls_add_new_link( $url, $keyword = '', $title = '' ) {
     // Allow plugins to short-circuit the whole function
     $pre = yourls_apply_filter( 'shunt_add_new_link', false, $url, $keyword, $title );
-    if ( false !== $pre )
+    if ( false !== $pre ) {
         return $pre;
+    }
 
-    $return = array();
-
+    /**
+     * The result array.
+     */
+    $return = [
+        // Always present :
+        'status' => '',
+        'code'   => '',
+        'message' => '',
+        'errorCode' => '',
+        'statusCode' => '',
+    ];
+
+    // Sanitize URL
     $url = yourls_sanitize_url( $url );
     if ( !$url || $url == 'http://' || $url == 'https://' ) {
         $return['status']    = 'fail';
         $return['code']      = 'error:nourl';
         $return['message']   = yourls__( 'Missing or malformed URL' );
-        $return['errorCode'] = '400';
+        $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+
         return yourls_apply_filter( 'add_new_link_fail_nourl', $return, $url, $keyword, $title );
     }
 
@@ -47,105 +64,18 @@ function yourls_add_new_link( $url, $keyword = '', $title = '' ) {
     yourls_check_IP_flood( $ip );
 
     // Prevent internal redirection loops: cannot shorten a shortened URL
-    if( yourls_get_relative_url( $url ) ) {
-        if( yourls_is_shorturl( $url ) ) {
-            $return['status']    = 'fail';
-            $return['code']      = 'error:noloop';
-            $return['message']   = yourls__( 'URL is a short URL' );
-            $return['errorCode'] = '400';
-            return yourls_apply_filter( 'add_new_link_fail_noloop', $return, $url, $keyword, $title );
-        }
+    if (yourls_is_shorturl($url)) {
+        $return['status']    = 'fail';
+        $return['code']      = 'error:noloop';
+        $return['message']   = yourls__( 'URL is a short URL' );
+        $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+        return yourls_apply_filter( 'add_new_link_fail_noloop', $return, $url, $keyword, $title );
     }
 
     yourls_do_action( 'pre_add_new_link', $url, $keyword, $title );
 
-    // duplicates allowed or new URL => store it
-    if( yourls_allow_duplicate_longurls() || !( $url_exists = yourls_long_url_exists( $url ) ) ) {
-
-        if( isset( $title ) && !empty( $title ) ) {
-            $title = yourls_sanitize_title( $title );
-        } else {
-            $title = yourls_get_remote_title( $url );
-        }
-        $title = yourls_apply_filter( 'add_new_title', $title, $url, $keyword );
-
-        // Custom keyword provided
-        if ( $keyword ) {
-
-            yourls_do_action( 'add_new_link_custom_keyword', $url, $keyword, $title );
-
-            $keyword = yourls_sanitize_keyword( $keyword, true );
-            $keyword = yourls_apply_filter( 'custom_keyword', $keyword, $url, $title );
-
-            if ( !yourls_keyword_is_free( $keyword ) ) {
-                // This shorturl either reserved or taken already
-                $return['status']  = 'fail';
-                $return['code']    = 'error:keyword';
-                $return['message'] = yourls_s( 'Short URL %s already exists in database or is reserved', $keyword );
-            } else {
-                // all clear, store !
-                try {
-                    yourls_insert_link_in_db( $url, $keyword, $title );
-                    $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => date('Y-m-d H:i:s'), 'ip' => $ip );
-                    $return['status']   = 'success';
-                    $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
-                    $return['title']    = $title;
-                    $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
-                    $return['shorturl'] = yourls_link($keyword);
-                } catch (Exception $e) {
-                    // Keyword supposed to be free but the INSERT caused an exception: most likely we're facing a concurrency problem
-                    $return['status']  = 'fail';
-                    $return['code']    = 'error:concurrency';
-                    $return['message'] = $e->getMessage();
-                    $return['errorCode'] = '503';
-                }
-            }
-
-        // Create random keyword
-        } else {
-
-            yourls_do_action( 'add_new_link_create_keyword', $url, $keyword, $title );
-
-            $timestamp = date( 'Y-m-d H:i:s' );
-            $id = yourls_get_next_decimal();
-            $ok = false;
-            do {
-                $keyword = yourls_int2string( $id );
-                $keyword = yourls_apply_filter( 'random_keyword', $keyword, $url, $title );
-                if ( yourls_keyword_is_free($keyword) ) {
-                    try {
-                        $insert = yourls_insert_link_in_db( $url, $keyword, $title );
-                        if ($insert){
-                            // everything ok, populate needed vars
-                            $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => $timestamp, 'ip' => $ip );
-                            $return['status']   = 'success';
-                            $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
-                            $return['title']    = $title;
-                            $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
-                            $return['shorturl'] = yourls_link($keyword);
-                        } else {
-                            // unknown database error, couldn't store result
-                            $return['status']   = 'fail';
-                            $return['code']     = 'error:db';
-                            $return['message']  = yourls_s( 'Error saving url to database' );
-                        }
-                    } catch (Exception $e) {
-                        // Keyword supposed to be free but the INSERT caused an exception: most likely we're facing a concurrency problem
-                        $return['status']  = 'fail';
-                        $return['code']    = 'error:concurrency';
-                        $return['message'] = $e->getMessage();
-                        $return['errorCode'] = '503';
-                    }
-                    $ok = true;
-                }
-                $id++;
-            } while ( !$ok );
-            @yourls_update_next_decimal( $id );
-        }
-
-    // URL was already stored
-    } else {
-
+    // Check if URL was already stored and we don't accept duplicates
+    if ( !yourls_allow_duplicate_longurls() && ($url_exists = yourls_long_url_exists( $url )) ) {
         yourls_do_action( 'add_new_link_already_stored', $url, $keyword, $title );
 
         $return['status']   = 'fail';
@@ -154,19 +84,84 @@ function yourls_add_new_link( $url, $keyword = '', $title = '' ) {
         $return['message']  = /* //translators: eg "http://someurl/ already exists" */ yourls_s( '%s already exists in database', yourls_trim_long_string( $url ) );
         $return['title']    = $url_exists->title;
         $return['shorturl'] = yourls_link($url_exists->keyword);
+
+        return yourls_apply_filter( 'add_new_link_already_stored_filter', $return, $url, $keyword, $title );
     }
 
-    yourls_do_action( 'post_add_new_link', $url, $keyword, $title, $return );
+    // Sanitize provided title, or fetch one
+    if( isset( $title ) && !empty( $title ) ) {
+        $title = yourls_sanitize_title( $title );
+    } else {
+        $title = yourls_get_remote_title( $url );
+    }
+    $title = yourls_apply_filter( 'add_new_title', $title, $url, $keyword );
+
+    // Custom keyword provided : sanitize and make sure it's free
+    if ($keyword) {
+        yourls_do_action( 'add_new_link_custom_keyword', $url, $keyword, $title );
+
+        $keyword = yourls_sanitize_keyword( $keyword, true );
+        $keyword = yourls_apply_filter( 'custom_keyword', $keyword, $url, $title );
 
-    if (isset($return['errorCode'])) {
-        $return['statusCode'] = $return['errorCode']; // there was a problem
+        if ( !yourls_keyword_is_free( $keyword ) ) {
+            // This shorturl either reserved or taken already
+            $return['status']  = 'fail';
+            $return['code']    = 'error:keyword';
+            $return['message'] = yourls_s( 'Short URL %s already exists in database or is reserved', $keyword );
+            $return['errorCode'] = $return['statusCode'] = '400'; // 400 Bad Request
+
+            return yourls_apply_filter( 'add_new_link_keyword_exists', $return, $url, $keyword, $title );
+        }
+
+        // Create random keyword
     } else {
-        $return['statusCode'] = 200; // regardless of result, this is still a valid request
+        yourls_do_action( 'add_new_link_create_keyword', $url, $keyword, $title );
+
+        $id = yourls_get_next_decimal();
+
+        do {
+            $keyword = yourls_int2string( $id );
+            $keyword = yourls_apply_filter( 'random_keyword', $keyword, $url, $title );
+            $id++;
+        } while ( !yourls_keyword_is_free($keyword) );
+
+        yourls_update_next_decimal($id);
+    }
+
+    // We should be all set now. Store the short URL !
+
+    $timestamp = date( 'Y-m-d H:i:s' );
+
+    try {
+        if (yourls_insert_link_in_db( $url, $keyword, $title )){
+            // everything ok, populate needed vars
+            $return['url']      = array('keyword' => $keyword, 'url' => $url, 'title' => $title, 'date' => $timestamp, 'ip' => $ip );
+            $return['status']   = 'success';
+            $return['message']  = /* //translators: eg "http://someurl/ added to DB" */ yourls_s( '%s added to database', yourls_trim_long_string( $url ) );
+            $return['title']    = $title;
+            $return['html']     = yourls_table_add_row( $keyword, $url, $title, $ip, 0, time() );
+            $return['shorturl'] = yourls_link($keyword);
+            $return['statusCode'] = 200; // 200 OK
+        } else {
+            // unknown database error, couldn't store result
+            $return['status']   = 'fail';
+            $return['code']     = 'error:db';
+            $return['message']  = yourls_s( 'Error saving url to database' );
+            $return['errorCode'] = $return['statusCode'] = '500'; // 500 Internal Server Error
+        }
+    } catch (Exception $e) {
+        // Keyword supposed to be free but the INSERT caused an exception: most likely we're facing a
+        // concurrency problem. See Issue 2538.
+        $return['status']  = 'fail';
+        $return['code']    = 'error:concurrency';
+        $return['message'] = $e->getMessage();
+        $return['errorCode'] = $return['statusCode'] = '503'; // 503 Service Unavailable
     }
 
+    yourls_do_action( 'post_add_new_link', $url, $keyword, $title, $return );
+
     return yourls_apply_filter( 'add_new_link', $return, $url, $keyword, $title );
 }
-
 /**
  * Determine the allowed character set in short URLs
  *
diff --git a/includes/functions.php b/includes/functions.php
index 06f75f86..7ad0915d 100644
--- a/includes/functions.php
+++ b/includes/functions.php
@@ -53,7 +53,7 @@ function yourls_get_next_decimal() {
  *
  * Note: this function relies upon yourls_update_option(), which will return either true or false
  * depending if there has been an actual MySQL query updating the DB.
- * In other words, this function may return false yet this would not mean it has functionnaly failed
+ * In other words, this function may return false yet this would not mean it has functionally failed
  * In other words I'm not sure we really need this function to return something :face_with_eyes_looking_up:
  * See issue 2621 for more on this.
  *