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

Dependancy loading change in 5.5 #22782

Closed
eithed opened this issue Jan 14, 2018 · 14 comments
Closed

Dependancy loading change in 5.5 #22782

eithed opened this issue Jan 14, 2018 · 14 comments

Comments

@eithed
Copy link

eithed commented Jan 14, 2018

  • Laravel Version: 5.5.28
  • PHP Version: 7.0.10
  • Affecting both Windows and Linux

Description:

With the recent upgrade from 5.4 to 5.5 we've noticed that phpunit started returning Error: No code coverage driver is available when trying to obtain code coverage report, even though xdebug is enabled in php.ini. Following debugging, we've noticed that when initialising Laravel application, dependancies are initialised (though they are not used in the given test) and the issue stems from using dependent Composer package. As far as I understand, with the upgrade the service providers in dependancies are now initialised when the App is initialised (although I might be wrong) - the issue on its own is replicable when installing larapack/voyager-hooks package. I don't know if the package is being initialised correctly to begin with, or how many other packages are following such initialisation steps/setup - it's still a change that I've not noticed in the upgrade guide (https://laravel.com/docs/5.5/upgrade).

I've created a topic on SO, where I've put in my thoughts on this issue: https://stackoverflow.com/questions/48118973/different-php-ini-file-loaded-dependant-upon-the-code-content/

Steps To Reproduce:

With testA within tests/unit/ATest.php displaying what php.ini file was loaded, with content of:

<?php

use Tests\TestCase;

class A extends TestCase
{
    public function testA()
    {
        echo( get_cfg_var('cfg_file_path')); exit;
    }
}

Beginning with clean installation of Laravel 5.5:

  • install larapack\voyager-hooks (composer require larapack/hooks)
  • run phpunit tests/unit/atest

Output: C:\Users[user]\AppData\Local\Temp\F2A5.tmp
Expected output: C:\server\php\php.ini

@eithed eithed changed the title Dependancy loading change Dependancy loading change in 5.5 Jan 14, 2018
@sisve
Copy link
Contributor

sisve commented Jan 14, 2018

How does this issue relate to laravel/framework? If the problem is in a larapack package, why is it reported here? What is the problem and what do you want to change?

@eithed
Copy link
Author

eithed commented Jan 14, 2018

The issue relates to Laravel as it's a change observable when moving from 5.4 to 5.5.
It's a problem that can be replicated by installing larapack/hooks package, but the issue in question might happen on any package, dependant, I guess, if there's a service provider within a package and how it is handled - as such it might affect a number of packages with unknown consequences. In 5.4 VoyagerHooksServiceProvider.php isn't automatically booted when initialising the App, in 5.5 it is.

@sisve
Copy link
Contributor

sisve commented Jan 15, 2018

The issue relates to Laravel as it's a change observable when moving from 5.4 to 5.5.

That doesn't make it Laravel's fault. You don't ask Microsoft for help when you upgrade to Windows 10 and some non-Microsoft application does something weird.

You've even told us (in comments on the Stack Overflow issue) that you cannot reproduce the problem with a clean version of Laravel 5.5. That is a clear indicator that the problem isn't at our end at all.

Do you understand that the problem you describe is a second php binary launched, with a custom php.ini file. Not that any service providers in packages will fail. No one else has bug reported this.

VoyagerHooksServiceProvider isn't part of laravel/framework. Laravel 5.5 introduced auto-loading support, which will load VoyagerHooksServiceProvider because ... larapack/voyager-hooks has told us to. Once again, the problem isn't in laravel/framework, but in a third party package.

@eithed
Copy link
Author

eithed commented Jan 15, 2018

Um... I would certainly flag this issue, if dynamically loaded libraries would now start loading differently.

You've even told us (in comments on the Stack Overflow issue) that you cannot reproduce the problem with a clean version of Laravel 5.5. That is a clear indicator that the problem isn't at our end at all.

I cannot indeed reproduce the issue with clear version of Laravel - this doesn't mean that the issue isn't with Laravel, as the interaction when loading the packages is the issue.

Do you understand that the problem you describe is a second php binary launched, with a custom php.ini file. Not that any service providers in packages will fail. No one else has bug reported this.

This is a consequence of the issue in question.

VoyagerHooksServiceProvider isn't part of laravel/framework. Laravel 5.5 introduced auto-loading support

Don't you think that this is problem? Maybe some packages are doing this (incorrectly? I don't know) - in such case it would make sense to say - "hey, package creators, now your service providers will be booted automatically when app initialises!", or "hey, when you upgrade to 5.5 check all your service providers as they will boot automatically when app initialises!" (although I don't know if it will do much good, I'd certainly not connected the dots). I've provided an instance where this becomes an issue - there might be other packages where this is an issue as well, albeit probably popping up differently.

@sisve
Copy link
Contributor

sisve commented Jan 15, 2018

Isn't it up to the packages to do it correctly? The functionality is an opt-in thing, packages have to explicitly tell us to auto-load their service providers via the composer.json file. If you believe that larapack/voyager-hooks is doing it wrong, file a bug report in their repository.

Can you show us the full stack trace for the original problem you reported?

@eithed
Copy link
Author

eithed commented Jan 15, 2018

I simply don't know if it's correct or not correct way the package is using the service provider. I know that the package behaviour hasn't changed, but that Laravel has changed the handling of service providers between the versions.

I can provide a stack trace of booting up VoyagerHooksServiceProvider, if that's what you're asking

@Miguel-Serejo
Copy link

Miguel-Serejo commented Jan 15, 2018

I know that the package behaviour hasn't changed, but that Laravel has changed the handling of service providers between the versions.

Which means the package needs to change the way it expects the service to be loaded. The change was documented, so package maintainers should know about this.

@eithed
Copy link
Author

eithed commented Jan 15, 2018

Excellent! Can you please provide where this change is documented?

@Miguel-Serejo
Copy link

https://laravel.com/docs/5.5/releases#laravel-5.5 scroll down to Package Discovery, which links to the documentation for package developers, which includes a section on package discovery: https://laravel.com/docs/5.5/packages#package-discovery. There's also a link to https://laravel.com/docs/5.5/providers which provides specific instructions for Providers.

@eithed
Copy link
Author

eithed commented Jan 15, 2018

Wouldn't it make sense to add a disclaimer on the https://laravel.com/docs/5.5/upgrade indicating that using package discovery in 5.5 is a breaking change? From what we've experienced it was "oh, we can upgrade, *going through list* nothing will change as far as we know"

@sisve
Copy link
Contributor

sisve commented Jan 15, 2018

You make it sound like the auto-discovery broke already working code, but that isn't the case. The package developers have to explicitly opt-in to use this new stuff. They did this in a commit dated 19 Oct 2017 indicating that they want the new auto-discovery. There was no auto-discovery in 5.4, and they had to change their code to have it in 5.5. Thus, the auto-discovery did not break anything since it's opt-in from the packages.

I also believe that the linked commit clearly disproves your argument "I know that the package behaviour hasn't changed", because that commit clearly shows that larapack/voyager-hooks opted in for the new stuff after it was released in august.

@36864 is phrasing it badly; there's no need for a package to change the way it loads services. They have to change their composer.json if they want to use the new auto-discovery stuff. They do not need to change anything if they want to keep using the old way where you edit config/app.php manually.

The 5.5 upgrade notes doesn't tell you how every other Laravel package may break. It's up to you as a developer to keep track of how things are setup and how packages expect them to be upgraded. If a package expects you to remove entries in config/app.php and use auto-discovery, then it is up to them to document it in their release docs.

Can you please stop acting like it's our fault that you have a problem that you cannot reproduce on a clean version of Laravel 5.5? We're trying to help you pinpoint the problem, but you're expecting a full apology from us without even providing us with stack traces that shows that it is a problem in our end, and you're totally blocking any way we attempt to help you to find out where the problem is.

@eithed
Copy link
Author

eithed commented Jan 15, 2018

@sisve I don't understand where I've asked for an apology or gave an impression that I want it - I want to resolve the issue in question, one way or another. If it's a problem with a package and the maintainer of the package is at fault, I'm fine with reporting it there. If it's an issue with how service providers are booted, or how the changes are documented, I'd assume you or whoever is using Laravel would want to know about it.

Can you please stop acting like it's our fault that you have a problem that you cannot reproduce on a clean version of Laravel 5.5?

I did not say that! It's reproducible per version change - install Laravel 5.4, install package in question, run the test; install Laravel 5.5, install package in question, run the test - the difference is there, with two clean installations of Laravel.

without even providing us with stack traces that shows that it is a problem in our end

I'll be happy to provide stack traces if you'll tell me what stack traces do you want. I've said:

I can provide a stack trace of booting up VoyagerHooksServiceProvider, if that's what you're asking

@sisve
Copy link
Contributor

sisve commented Jan 15, 2018

Report your issue at https://github.com/larapack/voyager-hooks/issues or https://github.com/larapack/hooks/issues, depending on who you think is the culprit. One of them has the auto-discover stuff, the other has integrations that executes additional php binaries and xdebug integrations.

If the problem doesn't exist without a package, and shows after you install a package, then it's most probably the package's fault.

@eithed
Copy link
Author

eithed commented Jan 15, 2018

@sisve will do - I wanted to report this on larapack/voyagey-hooks per this conversation.

We disagree on the matter, but I think at this point it's pointless to continue this discussion - thank you anyhow for your time!

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