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

Fixed leaking classes in autoloader #2000

Merged
merged 6 commits into from
May 2, 2024
Merged

Fixed leaking classes in autoloader #2000

merged 6 commits into from
May 2, 2024

Conversation

marekdedic
Copy link
Contributor

@marekdedic marekdedic commented May 29, 2023

Should close #1992 and possibly closes #1999 - @PeterSmith8nss and @kalich5, could you please check?
Closes #2524 , closes #1165

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (5b415c7) to head (6c89cd0).
Report is 6 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2000   +/-   ##
========================================
  Coverage      0.00%   0.00%           
  Complexity      391     391           
========================================
  Files            51      51           
  Lines          1656    1656           
========================================
  Misses         1656    1656           

@PeterSmith8nss
Copy link

I've done a quick test using the build artefact from "[Fixed leaking classes in autoloader #3654]". I still seem to be getting problems when my plugin and yours are both active. One case was when your code was looking for choose_handler(). I think we are getting a clash over which version of '/guzzlehttp/guzzle/src/functions.php' is loaded - because the identifier is determined from filename, not classname.

If I comment out the two guzzle files in arrays in my autoload_static.php and autoload_files,php it seems to work. It may be that the paths I've tested in my code are not using those functions and by commenting them out your code can link the file you want to those identifiers.

Would you avoid the problem if you renamed the guzzlehttp directory, e.g. sgdgguzzlehttp before building? You need some way to avoid your file having the same file_identifier as someone else using a different version/namespace.

@marekdedic
Copy link
Contributor Author

Hmm, can you please send me your plugin (or some minimal repro) to developer@dedic.eu? I'd like to be able to debug that...

@marekdedic marekdedic force-pushed the autoload-leak-fix branch 4 times, most recently from 2f27fa2 to 675fed4 Compare April 27, 2024 20:00
@marekdedic
Copy link
Contributor Author

Hi, I pushed some more changes that should fix the e-mailed issue. Also, this PR should fix #2524. @PeterSmith8nss , could you please check them? Thanks!

@marekdedic marekdedic mentioned this pull request Apr 29, 2024
@marekdedic marekdedic linked an issue Apr 29, 2024 that may be closed by this pull request
@PeterSmith8nss
Copy link

PeterSmith8nss commented Apr 30, 2024 via email

@marekdedic
Copy link
Contributor Author

@PeterSmith8nss They aren't in 2.13.10 - use this zip please (may have to rename to skaut-google-drive-gallery.zip): https://github.com/skaut/skaut-google-drive-gallery/actions/runs/8878119044/artifacts/1456462989

@PeterSmith8nss
Copy link

PeterSmith8nss commented Apr 30, 2024 via email

@marekdedic
Copy link
Contributor Author

Thanks, what you're seeing is an issue with insufficient PHP memory when serving a video file. By default, PHP only has 128MB, which is low for videos. So you can fix this by making the limit higher, but ideally, we should fix the plugin so that it doesn't eat all the memory...

Anyway, that's a separate issue, glad to hear the autoloader works for you, I'll merge this PR and do a new release soon-ish

@marekdedic
Copy link
Contributor Author

TODO: The scoper-autoload.php file references unprefixed globals and is included here:

require_once __DIR__ . '/vendor/scoper-autoload.php';

@marekdedic marekdedic merged commit 062fc61 into master May 2, 2024
10 checks passed
@marekdedic marekdedic deleted the autoload-leak-fix branch May 2, 2024 10:53
@marekdedic
Copy link
Contributor Author

@PeterSmith8nss Version 2.13.11 is out with the fix ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants