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

chore(macro): Deprecate global state in macro system #174

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,7 @@ members = ["crates/macros", "crates/cli"]

[package.metadata.docs.rs]
rustdoc-args = ["--cfg", "docs"]

[[example]]
name = "hello_world"
crate-type = ["cdylib"]
140 changes: 140 additions & 0 deletions NEW_MACROS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# New Macro Transition

The old macro system used a global state to be able to automatically register
functions and classes when the `#[php_module]` attribute is used. However,
global state can cause problems with incremental compilation and is not
recommended.

To solve this, the macro system has been re-written but this will require
changes to user code. This document summarises the changes.

## Functions

Mostly unchanged in terms of function definition, however you now need to
register the function with the module builder:

```rs
use ext_php_rs::prelude::*;

#[php_function]
pub fn hello_world() -> &'static str {
"Hello, world!"
}

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
module
.function(wrap_function!(hello_world))
}
```

## Classes

Mostly unchanged in terms of the class and impl definitions, however you now
need to register the classes with the module builder:

```rs
use ext_php_rs::prelude::*;

#[php_class]
#[derive(Debug)]
pub struct TestClass {
#[prop]
a: i32,
#[prop]
b: i32,
}

#[php_impl]
impl TestClass {
#[rename("NEW_CONSTANT_NAME")]
pub const SOME_CONSTANT: i32 = 5;
pub const SOME_OTHER_STR: &'static str = "Hello, world!";

pub fn __construct(a: i32, b: i32) -> Self {
Self { a: a + 10, b: b + 10 }
}

#[optional(test)]
#[defaults(a = 5, test = 100)]
pub fn test_camel_case(&self, a: i32, test: i32) {
println!("a: {} test: {}", a, test);
}

fn x(&self) -> i32 {
5
}

pub fn builder_pattern(
self_: &mut ZendClassObject<TestClass>,
) -> &mut ZendClassObject<TestClass> {
dbg!(self_)
}
}

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
module
.class::<TestClass>()
}
```

## Constants

The `#[php_const]` attribute has been deprecated. Register the constant
directly with the module builder:

```rs
use ext_php_rs::prelude::*;

const SOME_CONSTANT: i32 = 100;

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
module
.constant(wrap_constant!(SOME_CONSTANT)) // SOME_CONSTANT = 100
.constant(("CONST_NAME", SOME_CONSTANT)) // CONST_NAME = 100
}
```

## Extern

No changes.

```rs
use ext_php_rs::prelude::*;

#[php_extern]
extern "C" {
fn phpinfo() -> bool;
}

fn some_rust_func() {
let x = unsafe { phpinfo() };
println!("phpinfo: {x}");
}
```

## Startup Function

The `#[php_startup]` macro has been deprecated. Instead, define a function with
the signature `fn(ty: i32, mod_num: i32) -> i32` and provide the function name
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe explain what those numbers mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done a small explanation here

Copy link
Contributor

@ju1ius ju1ius Oct 26, 2023

Choose a reason for hiding this comment

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

Off the top of my head the module type can only be one of MODULE_PERSISTENT or MODULE_TEMPORARY, so it would be nicer to have a :

pub enum ModuleType {
    Persistent,
    Temporary,
}
impl From<i32> for ModuleType {
    fn from(value: i32) -> Self {
        match value => {
            ffi::MODULE_PERSISTENT => Persistent,
            ffi::MODULE_TEMPORARY => Temporary,
            _ => panic!("onoes!")
        }
    }
}
// TODO: impl From<ModuleType> for i32

and then:

fn startup(ty: ModuleType, id: i32) -> Result<(), SomeErrorType> {
    Ok(())
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Temporary remains a rare case, and then the default case is totally fine for now.
Let's open another PR, after merging it

to the `#[php_module]` attribute:
`mod_num` is the module number assigned by PHP for its internal management. The `ModuleBuilder` always defaults this parameter to `0`.
`ty` is the type of module (flag) which can be set to `MODULE_PERSISTENT = 0`.
The return number is for internal purposes and should always be `0` in this case. It's an internal mechanism to tell to the ModuleBuilder weither to use the defautlt startup function or the overriden one. (see: `crates/macros/src/module.rs:L35`)
Copy link
Contributor

@ju1ius ju1ius Oct 26, 2023

Choose a reason for hiding this comment

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

Are you shure of this ?

The module lifecycle callbacks are supposed to return a zend_result_code (either SUCCESS or FAILURE), so returning something like Result<(), ()> or Result<(), Box<dyn Error>> would be a much better choice IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be done in seperate PR, it will require some parsing here to enforce this signature


```rs
use ext_php_rs::prelude::*;

fn startup(ty: i32, mod_num: i32) -> i32 {
// register extra classes, constants etc
5.register_constant("SOME_CONST", mod_num);
Copy link
Contributor

Choose a reason for hiding this comment

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

typo ? i believe it should be in the get_module function

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think not. It's the global constant declaration here, and it's mostly used as a flag for the PHP end user
see:

/// Registers a global module constant in PHP, with the value as the content
/// of self. This function _must_ be called in the module startup
/// function, which is called after the module is initialized. The
/// second parameter of the startup function will be the module number.
/// By default, the case-insensitive and persistent flags are set when
/// registering the constant.
///
/// Returns a result containing nothing if the constant was successfully
/// registered.

Copy link
Contributor

@ju1ius ju1ius Oct 26, 2023

Choose a reason for hiding this comment

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

typo ? i believe it should be in the get_module function

No this code is correct.

The get_module function is used for registering functions, ini settings and module metadata (name, version, dependencies). Everything else must happen in startup (AKA MINIT), unless it need to be allocated per-request, in which case one must use request_startup (AKA RINIT).

EDIT: I was thinking in terms of the zend engine and forgot that this library actually lets you register everything inside get_module (by internally delaying the registration until startup). The code is correct nevertheless.

0
}

#[php_module(startup = "startup")]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
module
}
```
Loading
Loading