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

Cannot redeclare requireDependencies() #175

Closed
RobIsHere opened this issue Mar 22, 2017 · 25 comments
Closed

Cannot redeclare requireDependencies() #175

RobIsHere opened this issue Mar 22, 2017 · 25 comments

Comments

@RobIsHere
Copy link

Hi!

I get the following error sometimes. Somehow this is loaded twice. I installed with composer and I'm using the default autoloader and no manual require statements at all. So I have no idea how this can happen.

Cannot redeclare requireDependencies() (previously declared in libs/braintree/braintree_php/lib/Braintree.php:17) in libs/braintree/braintree_php/lib/Braintree.php:17

@juongithub
Copy link

Probably you are including twice the initialization script, how do you load the "autoload.php" script?

@RobIsHere
Copy link
Author

I should be protected from that if everything is working right here. That's what autoload does. Check if class XY exists yet and load it if it doesn't.

I have a lot of libraries included. Also e.g. all of laravel 5.1., mail service etc. That libraries work without errors.

@juongithub
Copy link

juongithub commented Mar 24, 2017

I think you are loading the library in the wrong way, or there was an error during the installation... I see the directory tree:

│   autoload.php   
├───bin       
├───braintree
│   └───braintree_php   
│       ├───lib
│       │   │   autoload.php
│       │   │   Braintree.php
│       [...]   
├───composer
├───phpunit         
├───symfony
└───vlucas

and I include the "autoload.php" in the root of the tree. Everything works fine for me.

@RobIsHere
Copy link
Author

I will investigate a little on this and get back to you. As I see this we need more information to get to a valid conclusion.

@RobIsHere
Copy link
Author

I extracted a minimal example that fails on the second "is_subclass_of".

<?php
include("vendor/autoload.php");

class XYZ {}
class ABC {}

if (is_subclass_of('Braintree', 'XYZ')) echo "subclass of xyz";
if (is_subclass_of('Braintree', 'ABC')) echo "subclass of abc";

Exception:

PHP Fatal error:  Cannot redeclare requireDependencies() (previously declared in /root/projects/braintest/vendor/braintree/braintree_php/lib/Braintree.php:16) in /root/projects/braintest/vendor/braintree/braintree_php/lib/Braintree.php on l
ine 23                                                                                                                                                                                                                                          
PHP Stack trace:                                                                                                                                                                                                                                
PHP   1. {main}() /root/projects/braintest/test.php:0                                                                                                                                                                                           
PHP   2. is_subclass_of() /root/projects/braintest/test.php:8                                                                                                                                                                                   
PHP   3. spl_autoload_call() /root/projects/braintest/test.php:8                                                                                                                                                                                
PHP   4. Composer\Autoload\ClassLoader->loadClass() /root/projects/braintest/test.php:8                                                                                                                                                         
PHP   5. Composer\Autoload\includeFile() /root/projects/braintest/vendor/composer/ClassLoader.php:322                                                                                                                                           

Tested on PHP 5.6.30 and 7.
braintest.tar.gz

@juongithub
Copy link

Braintree is not a class, but the root namespace. With those line of codes you are forcing the "autoloader" to load the file Braintree.php that does not contain any class definition. You will see the same error with these lines:

include("vendor/autoload.php");
echo class_exists( 'Braintree' );
echo class_exists( 'Braintree' );

these lines do not cause the error:

include("vendor/autoload.php");
echo class_exists( 'Braintree\Configuration' );
echo class_exists( 'Braintree\Configuration' );

It is an irrelevant issue since you never need to instantiate Braintree (that is not a class and therefore cannot be instantiated). However, if you want to solve this just add:

class Braintree {}

anywhere in Braintree.php.

@RobIsHere
Copy link
Author

Omg! Is it so hard to wrap this "requireDependencies" in function_exists?

Yes this Issue has 3 Sources:

  1. I made a const PAYMENT_PROVIDER
  2. Rethinkdb has maybe a Bug checking is_subclass_of of everything including
  3. You have a Bug because you are not following the psr autoloader specifications! That you are defining a function there is simply NOT ALLOWED!
class User {
    const PAYMENT_PROVIDER = 'Braintree';
    ....
}

Saving the user with rethinkdb in a totally unrelated area does the is_subclass check for every property
https://github.com/danielmewes/php-rql

@RobIsHere
Copy link
Author

I fixed the Issue in my code. For me there is no problem anymore. All others saving something with the name "Braintree" on it using rethinkdb will have this problem. But that's up to you, I have done enough.

@juongithub
Copy link

juongithub commented Apr 5, 2017

You can always open a "pull request."

@RobIsHere
Copy link
Author

No, I'm done here.

@crookedneighbor
Copy link
Contributor

@RobIsHere @juongithub Thanks for investigating this issue.

I'm concerned about your assertion that we are not following the PSR autoloader. The Braintree team will evaluate and see if we need to make an update to the SDK.

@juongithub
Copy link

Actually, it seems to me that psr specifications require a "root namespace" (i.e., a vendor name) but do not say nothing about the existence of a class with the same name.

@RobIsHere
Copy link
Author

@crookedneighbor here are some pointers...

http://www.php-fig.org/psr/psr-4/#overview
This PSR describes a specification for autoloading classes from file paths

http://www.php-fig.org/psr/psr-4/#specification Point 1 defines the supported structures:
classes, interfaces, traits, and other similar structures

Note the absence of function. So the autoloader is probably not prepared for this.

Breaking examples on the web:
https://wordpress.org/support/topic/500-error-php-fatal-error-cannot-redeclare-requiredependencies/
https://wordpress.org/support/topic/fatal-error-paid-membership-pro/

(I made the experience that following specifications to the point makes my software more reliable. So I personally would remove the file and search for another place to do the check. But thats definitely more work and only my opinion, a function_exists check should be good enough in practice)

@juongithub
Copy link

juongithub commented Apr 5, 2017

autoloader is not prepared to load a root namespace as a class :)

@RobIsHere
Copy link
Author

*rofl :)

@RobIsHere
Copy link
Author

danielmewes/php-rql#155

@EvanHahn
Copy link
Contributor

EvanHahn commented Apr 5, 2017

@RobIsHere We took another look at this and want to make sure we understand your issue.

It seems like lib/Braintree.php is being double-required in a few places, which is causing problems because that redeclares a function called requireDependencies. This redeclaration may be a violation of the PSR-4 autoloading specification, but in any case, this is probably something worth fixing.

Do we understand your problem?

If so, we were thinking of changing the file to look like this:

if (!function_exists('requireDependencies')) {
  function requireDependencies() {
    $requiredExtensions = ['xmlwriter', 'openssl', 'dom', 'hash', 'curl'];
    foreach ($requiredExtensions AS $ext) {
      if (!extension_loaded($ext)) {
        throw new Braintree_Exception('The Braintree library requires the ' . $ext . ' extension.');
      }
    }
  }

  requireDependencies();
}

If it's not too much trouble, could you try updating your lib/Braintree.php with that to see if it fixes your problem? If it does, we'll investigate a little further and update our library if things look good to all of us.

@RobIsHere
Copy link
Author

My code looks like your code but with 4-spaces tabs :)

@EvanHahn
Copy link
Contributor

EvanHahn commented Apr 5, 2017

@RobIsHere To clarify—did you make this change and did it work for you?

@juongithub
Copy link

juongithub commented Apr 5, 2017

Actually, the proposed solution is only a workaround, the root problem is that the autoloader thinks that the class Braintree is defined in the lib/Braintree.php file. The first time the class Braintree is required the autoloader loads the file lib/Braintree.php. The second time the class Braintree is required, the autoloader reload the lib/Braintree.php file since the class Braintree still does not exist, thus generating the error.

There are several ways to solve this problem, but, in my opinion, a more coherent way should be preferred. For example, lib/Braintree.php could be renamed to lib/init.php, since Braintree is the root namespace and not a class. Furthermore, lib/Braintree.php is actually an initialization script.

@EvanHahn
Copy link
Contributor

@juongithub The problem with that is that it's a breaking change for some people. If we rename the files, then this documentation will be wrong:

require_once 'PATH_TO_BRAINTREE/lib/Braintree.php';

Do you know of a way to tell the autoloader about this difference? Or is the only solution to this problem a major version bump of the library?

In any case: @RobIsHere does this workaround work for you?

@juongithub
Copy link

@RobIsHere I think you can change the autoload "psr-0" settings in composer.json from:

"autoload": {
        "psr-0": {
            "Braintree": "lib/"
        },
        "psr-4": {
            "Braintree\\": "lib/Braintree"
        }
    }

to:

"autoload": {
        "psr-0": {
            "Braintree": "lib/Braintree"
        },
        "psr-4": {
            "Braintree\\": "lib/Braintree"
        }
    }

this solves the problem in my local machine.

@RobIsHere
Copy link
Author

Oh I missed that message. The upper solution is running and it works. Until the next update we will solve it, I'm sure!

@RobIsHere
Copy link
Author

I don't want to do too much try and error because the problem only hit me on production. So I'm careful, I hope you understand that.

That's how I would solve it: The responsibility to check the system configuration can go into the Configuration class. I would hook it somewhere early enough. Maybe that would be possible?

@juongithub
Copy link

I understand your point, in my opinion the best ways to solve the problem are (ordered from the most consistent to the least consistent):

  1. change the file name from Braintree.php to init.php
  2. modify the psr-0 in composer.json from "Braintree": "lib/" to "Braintree": "lib/Braintree"
  3. add the line class Braintree {} to Braintree.php
  4. add if (!function_exists('requireDependencies')) {...} to Braintree.php

However, I would avoid solution 4 since it does not prevent the autoloader to indefinitely re-load the file Braintree.php (i.e., the autoloader will reload the file Braintree.php every time the code requests the class Braintree); this does not happen with solution 3 since the class Braintree is defined the first time the file Braintree.php is loaded.

quinnjn pushed a commit to quinnjn/braintree-php-issue-175 that referenced this issue Apr 10, 2019
hollabaq86 pushed a commit that referenced this issue May 12, 2020
hollabaq86 pushed a commit that referenced this issue May 12, 2020
…d-tests

BTOPTMZ-164 remove sepa related tests
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

4 participants