Skip to content

[WIP] Packages #4490

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

Closed
wants to merge 1 commit into from
Closed

[WIP] Packages #4490

wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 1, 2019

Packages are registered using the package_declare() function (which should be called by composer in the future, based on composer.json information):

<?php
package_declare([
    "name" => "nikic/php-parser",
    "declares" => [
        "strict_types" => 1,
    ],
]);

Files then specify which package they are part of:

<?php
package "nikic/php-parser";
// ...

The package concept is orthogonal to namespaces. In this patch it is used only to manage declares for a whole package. In the future it may be used to implement package-private visibility, or any other functionality that is supposed to be package-specific.

This is an alternative to https://wiki.php.net/rfc/namespace_scoped_declares (#2972).

TODO:

  • Preloading support.

@brzuchal
Copy link
Contributor

brzuchal commented Aug 3, 2019

IMO packages should have FQN and be declared and defined using FQN. Using strings looks weird cause next declaration after package would be namespace with FQN.

@nikic
Copy link
Member Author

nikic commented Aug 3, 2019

@brzuchal I avoided FQN specifically because packages (in this implementation!) have nothing to do with name resolution, and using an FQN would make it look like they do. A simple string allows directly using existing Composer package names, instead of having to invent some kind of mapping.

include __DIR__ . "/php_cli_server.inc";
php_cli_server_start(getenv('TEST_PHP_EXTRA_ARGS'));

echo file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS . "/packages_invalidation_with_strict_types.php" );
Copy link
Member

@petk petk Aug 3, 2019

Choose a reason for hiding this comment

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

Hello, worth noting that tests/*.php files are gitignored... Some editors, and IDEs have issue with this and recommend unignoring these. And for possible automated tools it is simpler to not add ignored files in the repo. So, maybe a simple rename of /tests/.php -> /tests/.inc could be a solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

These tests use the built-in server, which doesn't support .inc as a file-extension for PHP files.

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe we can invent a better file extensions naming strategy here? For example, either tests output files would be simple .php (a bit misnamed strategy from the beginning but let's have it now), and such "code" or included files would be .inc.php or something? That way they can be ignored and tracked like this in gitignore file:

**/tests/**/*.php
!**/tests/**/*.inc.php

@brzuchal
Copy link
Contributor

brzuchal commented Aug 12, 2019

One thing I would like to mention is that given approach would be a second way of declaring compiler declares. On files with mixed HTML content together with PHP code a declare directive would still be in use so even if we push declare directives for classes, functions and constants in one place we will still declare it multiple times if package contains files with mixed content.

@@ -2079,6 +2081,24 @@ zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type)
}
}

/* If this script is part of a package, make sure the package declaration did not change. */
if (persistent_script && persistent_script->package &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly in this block, if package_declare() changes, then the script gets re-compiled?

The case of composer is pretty straightforward, as the package declares would be generated into vendor/autoload.php or a subsequent file.

But technically it is possible to call package_declare dynamically with values that change every time the declaration is executed: package_declare(['name' => 'foo/bar', 'declares' => ['strict_types' => random_int(0, 1)]]);

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. Things like ticks or strict_types influence the way code is compiled, so we can't reuse the compilation if the package declaration changed.

It would be possible to instead cache multiple versions of the same file and chose between them. However, in practice the package declaration will never change (or if it changes, then the old version will never be used again), as such it is more efficient to only cache one version and verify that things are still the same.

@beberlei
Copy link
Contributor

beberlei commented Aug 14, 2019

With respect to IDE integration I could see one downside with this approach. If Composer were to generate the package_declare calls, then IDEs could only see how to parse the files after the definitions have been generated, so when composer dump-autoload was called.

For applications with even more dynamic package loading (Wordpress or any CMS, E-Commerce system with plugins really), the declares might even come from a database table. So an IDE would never see them.

I believe a solution where the package declaration is "autoloaded" by traversing up from the first declaration of package to search the euiqvalent of Python's __init__.py, i.e. __init__.php which then includes the call to package_declare would be more robust.

Edit i know you mentioned this as point 4 here: https://externals.io/message/101323#106345 - and I don't like the implicit loading myself, but I think the robustness is a strong enough argument over declaring it in composer.json

@nikic
Copy link
Member Author

nikic commented Aug 14, 2019

With respect to IDE integration I could see one downside with this approach. If Composer were to generate the package_declare calls, then IDEs could only see how to parse the files after the definitions have been generated, so when composer dump-autoload was called.

For applications with even more dynamic package loading (Wordpress or any CMS, E-Commerce system with plugins really), the declares might even come from a database table. So an IDE would never see them.

There are basically two cases: a) Using composer, in which case declares should live in composer.json and IDEs would recognize that or b) some kind of manual scheme, in which case they should pass a static array to package_declare() if they want IDE support. If they pass something dynamic (which makes, for the record, absolutely zero sense) then there's nothing that can be done in any case.

I believe a solution where the package declaration is "autoloaded" by traversing up from the first declaration of package to search the euiqvalent of Python's __init__.py, i.e. __init__.php which then includes the call to package_declare would be more robust.

Edit i know you mentioned this as point 4 here: https://externals.io/message/101323#106345 - and I don't like the implicit loading myself, but I think the robustness is a strong enough argument over declaring it in composer.json

See https://externals.io/message/101323#106365. I don't think anything that involves paths or directories is a good idea where PHP is concerned.

@beberlei
Copy link
Contributor

There are basically two cases: a) Using composer, in which case declares should live in composer.json and IDEs would recognize that

This would require IDEs to know both composer.json and manual declaration of package_declare. Composer is sufficiently wide spread that warrants it, but maybe IDEs only want to implement the PHP code based definitions.

or b) some kind of manual scheme, in which case they should pass a static array to package_declare() if they want IDE support. If they pass something dynamic (which makes, for the record, absolutely zero sense) then there's nothing that can be done in any case.

i agree it makes no sense, but given that package_declare is a function it would technically be possible to do it dynamically.

See https://externals.io/message/101323#106365. I don't think anything that involves paths or directories is a good idea where PHP is concerned.

I remember we talked about it before, sorry. I rest my case then.

@iluuu1994
Copy link
Member

Closing as this seems inactive and there's no associated RFC.

@iluuu1994 iluuu1994 closed this Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants