chore(macro): Deprecate global state in macro system

[davidcole1340]

closes #131

[azjezz]

@davidcole1340 what is the status here? anything i can do to help? :)

[ptondereau]

@davidcole1340 what is the status here? anything i can do to help? :)

Testing testing and testing haha. From the author, this is mainly finished

[azjezz]

I have actually tested it a bit today :D seems good to me

[ju1ius]

I did a bit of basic tests and so far your PR solves one of the big pain points I have with the current macro system: When you have a modestly large project with some submodules, you can not register functions/classes without having to star-import your subodule into the main lib file.

With this PR, now it is possible to do this:

// lib.rs
mod sub;

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
    sub::register(module)
}

// sub/mod.rs

#[php_function]
fn hello_sub(name: &str) -> String {
    format!("Hello sub, {}!", name)
}

pub fn register(module: ModuleBuilder) -> ModuleBuilder {
    module.function(wrap_function!(hello_sub))
}

without having to import all the things into the main module, so big 👍🏻 on this !

[ju1ius]

The build fails when the closure feature is enabled.

error[E0046]: not all trait items implemented, missing: `BUILDER_MODIFIER`, `EXTENDS`, `IMPLEMENTS`, `method_builders`, `constructor`, `constants`
   --> /ext-php-rs/src/closure.rs:152:1
    |
152 | impl RegisteredClass for Closure {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `BUILDER_MODIFIER`, `EXTENDS`, `IMPLEMENTS`, `method_builders`, `constructor`, `constants` in implementation
    |
   ::: /ext-php-rs/src/class.rs:28:5
    |
28  |     const BUILDER_MODIFIER: Option<fn(ClassBuilder) -> ClassBuilder>;
    |     ---------------------------------------------------------------- `BUILDER_MODIFIER` from trait
...
31  |     const EXTENDS: Option<fn() -> &'static ClassEntry>;
    |     -------------------------------------------------- `EXTENDS` from trait
...
34  |     const IMPLEMENTS: &'static [fn() -> &'static ClassEntry];
    |     -------------------------------------------------------- `IMPLEMENTS` from trait
...
60  |     fn method_builders() -> Vec<(FunctionBuilder<'static>, MethodFlags)>;
    |     --------------------------------------------------------------------- `method_builders` from trait
...
63  |     fn constructor() -> Option<ConstructorMeta<Self>>;
    |     -------------------------------------------------- `constructor` from trait
...
66  |     fn constants() -> &'static [(&'static str, &'static dyn IntoZvalDyn)];
    |     ---------------------------------------------------------------------- `constants` from trait

[ptondereau]

Green CI here! Maybe have a second round of review @ju1ius @joehoyle @joelwurtz @danog

[joelwurtz]

EDIT: after looking again, this is not how it works, so let's forget this comment

I was in fact looking into doing that in a different way as the current and this macro system does not support having a module load in embed. Let me explain :

PHP rely on a zend_module_entry struct for it's module. When writing an extension in C, it's usually done by writing a static object into the root and add an extern on it.

There is also a get_module function that returns a pointer to this struct, which allows to know how to load a module when using dynamic loading

In ext-php-rs, only the get_module function is declared which means that the extension can only be load if it's used as a dynamic module (you can see there is a hacks in current test method which calls this function when launching an Embed, but unfortunately this hacks does not work for classes as PHP already have loaded thoses classes)

My goal was to write a macro which would mimic the way of PHP, something like :

php_module! {
    startup_function = my_startup_fonction,
    name = "extension-name",
    ...
};

Which would done the following :

pub extern "C" const module_extension: zend_module_entry = zend_module_entry {
    ...
}

#[cfg(lib)]
pub extern "C" fn get_module() -> *const zend_module_entry {
    return &zend_module_entry;
}

This way the get_module will only be available when compiled as a lib but not as a binary where it would be loaded because there is an extern on the zend_module_entry

Let me know if this way is preferred, i have made some work on it but it's not ready yet

[ju1ius]

@joelwurtz I'm not sure I understand...

I have the feeling that you're confusing modules with extensions.

Modules are loaded from ini entries (extension=libfoo.so) and must export a get_module symbol which must be a function returning a zend_module_entry pointer.

Extensions are loaded from ini entries (zend_extension=libbar.so) and must export a zend_extension_entry symbol which must be a statically allocated zend_extension struct.

So to me it sounds like what you'd like to implement is the hybrid extension pattern described in the book.

Or maybe I'm just missing something, in which case would you mind clarifying? 😉

[joelwurtz]

I was correctly talking about a PHP extension (zend_module_entry) but i misunderstood how sapi (and so embed) loaded themself as a PHP extension (so you could build a php binary with extra functions)

But in fact this is done when using the php_module_startup function (https://github.com/php/php-src/blob/master/main/main.c#L2027) which takes an sapi module and add optional zend_module_entry pointer (so it can register a new extension when starting up)

As an example this is done by php-fpm to register it's own functions (like it's shutdown one) https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L774

So disregard my comment it's off topic for this PR, sorry for the noise

I was confused on how PHP builtin extension (like date or other ones) were loaded (when they are not a shared file), and in fact it's just a template header file that is computed during make which list all extensions that will be compiled directly inside binaries (php cli / fpm / embed / etc ...) see https://github.com/php/php-src/blob/master/main/internal_functions.c.in

[ju1ius]

I was correctly talking about a PHP extension

Yes the fact that these terms are mixed and matched in the source PHP source code and documentation is rather unfortunate (mostly due to legacy I guess) and doesn't ease communication. I personnaly think that «modules vs extensions» is clearer than «PHP extensions vs Zend extensions», but to each his own I guess.

WRT the actual PR

Sorry, I don't have the bandwidth to make an exhaustive review but from what I can remember from last year it was a clear step forward to me, so I'm all in favour of merging it while it's hot !