Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lordofweb opened this issue Jul 16, 2024 · 2 comments
Open

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

lordofweb opened this issue Jul 16, 2024 · 2 comments

Comments

@lordofweb
Copy link

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
Copy link

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
Copy link
Author

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:

  • The PHP 8.1+ compatibility issue
  • The "Undefined global variable $version" warning
  • The constant warnings you were experiencing in the constructor

You can find the updated PR here: #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.

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

No branches or pull requests

2 participants