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

Fix psr4 autoload warning #322

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Fix psr4 autoload warning #322

merged 1 commit into from
Nov 18, 2020

Conversation

ChaerilM
Copy link
Contributor

composer gives warning while update. in some case, it does not add lavachart to autoloading due to this issue

@coveralls
Copy link

coveralls commented Mar 26, 2020

Coverage Status

Coverage remained the same at 69.207% when pulling bed74ac on Zabanya:3.1 into e3b719c on kevinkhill:3.1.

Copy link
Owner

@kevinkhill kevinkhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, but we can't just change a class name in it's defining file and not everywhere else it is located in the project.

@ChaerilM
Copy link
Contributor Author

ChaerilM commented May 4, 2020

i could not find any other file that uses Khill\Lavacharts\Support\Renderable nor Khill\Lavacharts\Support\RenderableTrait

@kevinkhill
Copy link
Owner

So I have a useless trait causing issues... I will give a second look when I get a chance. Thank you for bringing this to my attention

@ChaerilM
Copy link
Contributor Author

ChaerilM commented May 5, 2020

it might be an uncleaned commit after migration since Khill\Lavacharts\Support\Renderable and Khill\Lavacharts\Support\Traits\RenderableTrait had similar structure

@kevinkhill
Copy link
Owner

It actually looks like you uncovered my indecisiveness in choosing inheritance over composition with traits... I think Renderable used to be a trait, but then I made it a class...

investigating...

kevinkhill added a commit that referenced this pull request Sep 9, 2020
@kevinkhill
Copy link
Owner

I also moved the 4.0 work into its own branch so master tracks the current development of 3.1 again

@schonhose schonhose mentioned this pull request Nov 18, 2020
@kevinkhill kevinkhill merged commit f4298d9 into kevinkhill:3.1 Nov 18, 2020
@yaayes
Copy link

yaayes commented Nov 21, 2020

Hi thanks for the PR, but I still see this warning when running composer update

Class Khill\Lavacharts\Support\RenderableTrait located in ./vendor/khill/lavacharts/src/Support/Renderable.php does not comply with psr-4 autoloading standard. Skipping.

@yaayes
Copy link

yaayes commented Nov 21, 2020

Can you please tag a patch release to resolve this issue, thank you! cc @kevinkhill
https://github.com/kevinkhill/lavacharts/compare/3.1.13..3.1

@yaayes
Copy link

yaayes commented Nov 21, 2020

I forget to mention that I have Composer version 2 (moved to it because it's much faster), so this is an issue now not just a warning!

@ChaerilM
Copy link
Contributor Author

@yaayes I believe he push the patch for version 4, I've tried it but seems there was a breaking changes and I could not find doc for it. for now probably wait in indefinite time

@kevinkhill
Copy link
Owner

awww, I don't like discouraging you and hearing "indefinite" I have 30 minutes right now, lets see if I can make something happen.

@yaayes
Copy link

yaayes commented Nov 23, 2020

Hi thankyl you @ChaerilM and @kevinkhill
for the response. Why not a patch for the version 3, it will not break anything since as @ChaerilM says that trait is not used anywhere in the package

@kevinkhill
Copy link
Owner

@kevinkhill
Copy link
Owner

kevinkhill commented Nov 23, 2020

let me know if errors arise please! I didn't test anything, because I'm off to my paying job now 😄

@yaayes
Copy link

yaayes commented Nov 23, 2020

Thanks @kevinkhill for taking the time to solve this. Very appreciated!

@yaayes
Copy link

yaayes commented Nov 23, 2020

Updated successfully without any warnings related to composer v2, and I can confirm that nothing break (at least in my application which use like four/five type of commons charts)

@kevinkhill
Copy link
Owner

Awesome, glad to hear that. 🤞🏻 All my hot glue and duct tape keeps 3.1 together until I eveeeeentually carve time for v4

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

Successfully merging this pull request may close these issues.

4 participants