Skip to content

PHPLIB-1208: Split example namespaces #1139

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

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Jul 24, 2023

PHPLIB-1208

The appstract/laravel-opcache package looks compiles all php files using opcache_compile_file. This creates some duplicate function declarations in our example files. Since we want to ship the examples for users' benefit, these files need to be excluded by another measure. Luckily, the package excludes files containing a shebang, so adding that and making the examples executable not only fixes the opcache issues, but also makes them a little easier to run.

The above solution does not work on all PHP versions, as our tests require the files and the addition of the shebang makes the declare and namespace declarations no longer the first statements in the file. To work around the conflicts observed when compiling for opcache, the examples now use separate namespaces.

@alcaeus alcaeus requested a review from GromNaN July 24, 2023 05:52
@alcaeus alcaeus self-assigned this Jul 24, 2023
@alcaeus alcaeus force-pushed the phplib-1208-make-examples-executable branch from a197992 to 2b95e64 Compare July 24, 2023 06:34
@alcaeus alcaeus changed the title PHPLIB-1208: Make examples executable PHPLIB-1208: Split example namespaces Jul 24, 2023
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

If nothing else, it's not bad.

IMO, the laravel package should have used the autoload paths from the composer config of each package.

I submitted appstract/laravel-opcache#143 to exclude "examples" directory, that would benefit for all libraries that provide such examples.

@alcaeus alcaeus merged commit b252443 into mongodb:v1.16 Jul 24, 2023
@alcaeus alcaeus deleted the phplib-1208-make-examples-executable branch July 24, 2023 07:21
@@ -1,7 +1,7 @@
<?php
declare(strict_types=1);

namespace MongoDB\Examples;
namespace MongoDB\Examples\Aggregate;
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to have the same problem with the CSFLE examples? Or perhaps not because those don't define any functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially, yes. In this particular case we won't, however I consider this an upstream issue, as blindly compiling every PHP file regardless of whether it's even available to be autoloader is an error. The PR @GromNaN created in the library excludes all files in examples folders from compilation, which fixes the issue for good. This is just a temporary workaround to resolve the immediate issue until it can be fixed upstream.

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.

3 participants