getbrevo / brevo-php

A fully-featured PHP API client to interact with Brevo.
https://developers.brevo.com/
MIT License
48 stars 21 forks source link

Global variable conflict: Rename 'version' to 'version-brevo' #31

Open lordofweb opened 2 months ago

lordofweb commented 2 months ago

Hello,

I've encountered an issue with the global variable named "version" in the Brevo PHP package. This global variable is causing conflicts in my project.

To resolve this, I propose changing the variable name to something more specific to the Brevo package. This would help avoid potential conflicts with other packages or user-defined variables.

Here's a proposed patch to address this issue:

--- a/lib/Configuration.php
+++ b/lib/Configuration.php
@@ -38,7 +38,7 @@
  * @link     https://github.com/swagger-api/swagger-codegen
  */

-$GLOBALS['version'] = '2.0.0';
+$GLOBALS['version-brevo'] = '2.0.0';

 class Configuration
 {
@@ -120,7 +120,7 @@
     public function __construct()
     {
         $this->tempFolderPath = sys_get_temp_dir();
-        $this->userAgent = 'brevo_clientAPI/v' . $GLOBALS['version'] . '/php'; 
+        $this->userAgent = 'brevo_clientAPI/v' . $GLOBALS['version-brevo'] . '/php';
     }

This change makes the variable name more specific to the Brevo package, reducing the likelihood of conflicts with other code. Please review this pull request and let me know if you need any further information or if you'd like me to make any adjustments to this proposed change. Thank you for your consideration.

jasperpoppe commented 4 days ago

As of PHP 8.1.0, write access to the entire $GLOBALS array is no longer supported: https://www.php.net/manual/en/reserved.variables.globals.php

I think there should be another solution to set the userAgent.

lordofweb commented 4 days ago

Thank you @jasperpoppe for raising this important issue. I fully agree with your concerns regarding the use of $GLOBALS, especially given the changes in PHP 8.1.0.

I've updated the Pull Request to address this issue comprehensively. The new solution completely eliminates the use of $GLOBALS, resolving both the compatibility issue with PHP 8.1+ and the warning you've been experiencing.

Here's what the updated PR does:

  1. Removes the line $GLOBALS['version'] = '2.0.0'; entirely.
  2. Introduces a VERSION constant in the Configuration class.
  3. Updates the userAgent string in the constructor to use this constant instead of $GLOBALS.

Here's a snippet of the change:

class Configuration
{
    /**
     * Version of the package
     */
    const VERSION = "2.0.0";

    // ...

    public function __construct()
    {
        $this->tempFolderPath = sys_get_temp_dir();
        $this->userAgent = 'brevo_clientAPI/v' . self::VERSION . '/php';
    }
}

This change should resolve:

You can find the updated PR here: https://github.com/getbrevo/brevo-php/pull/32

I believe this solution effectively addresses the concerns you raised in issue #43. It gets rid of the $GLOBALS['version'] variable as you suggested, while maintaining the functionality.

Please review these changes when you have a chance and let me know if you think any further modifications are needed. Your feedback on this solution would be greatly appreciated.