Open faassen opened 3 days ago
I've delved into the code somewhat but I still don't have a fix.
In crates/macros/src/module.rs
there's an impl Describe
which generates the implements information that is then, I think, later used by cargo php stubs
to generate the stubs.
The interface information takes the form of strings like "ce::accesscontrol()" which are then literally quoted into the output, as strings. That's not going to work - we want the string "AccessControl" instead.
I tried adding an import to zend::ce
in the generated code, then the following:
let interfaces = self.interfaces.iter().map(|iface| {
let iface: TokenStream = iface.parse().unwrap();
quote! { #iface.name().unwrap().into() }
});
First I parse code like ce::accesscontrol()
to a TokenStream. Then I include that call literally. From it, I should get a ClassEntry
which has a name
method which returns an Option<&str>
.
Unfortunately when I try this the classes seem to be nameless and cargo php stubs
fails the Option
unwrap.
I'm confused as if I go into the PHP C code the name does seem to be set for ClassEntry
. I'll do more digging but I thought I'd leave a report here first so others interested may be able to fllow allow.
Hm, I was wrong about the unwrap. It somehow takes place in
pub fn arrayaccess() -> &'static ClassEntry {
unsafe { zend_ce_arrayaccess.as_ref() }.unwrap()
}
So perhaps this code is running before these C values are fully initialized somehow.
There seems to some module startup code that initializes these classes, so apparently that's not being run during stub generation.
I looked at introducing the right PHP class names like ArrayAccess
and Countable
earlier on, but unfortunately that literally takes the tokenstream from the #[implements
macro so that's going to be ce::arrayaccess()
and such.
I think there are two possible ways forward:
the nicest way would be to somehow initialize the names of these PHP things during stub generation. I think the modifications I made would then work. Unfortunately I don't know how to do that yet.
a hacky way would be to create a hardcoded match to recognize strings like ce:arrayaccess()
and replace it with "ArrayAccess". But that wouldn't be very maintainable.
I might have found the reason. Will need to do some testing. Will report back.
@xenira Hey! Glad to see someone else is looking into this! If you think it would help to chat on discord or something to debug this together, let me know!
Small update. Root cause is that the string was not converted back into an syn::Expr
before writing it. That caused it to be interpreted as a string literal.
Changing this revealed some more underlying issues.
Well, guess now that I think about it I did the same thing as you. Also failing at the unwrap now...
So as far as I understand it the problem is, that the extension is not loaded inside a PHP context when generating stubs. That causes the ClassEntry
to not beeing available.
In addition to @faassen s suggestions maybe do the following:
ClassEntry::try_find
This would shift some validation to the startup logic, instead when generating stubs. But as this seems to be the case already with string literals beeing used it should be fine for now.
Edit: Well, after testing it this does not seem to work, as the class entry is not found on startup...
Edit2: ClassEntry::try_find
/zend_lookup_class_ex
seems to be the wrong approach. Will try something different tomorrow
I guess it's not possible to make the PHP context exist using cargo php stubs
?
By saying "use the interface name" you mean this?
#[implements(ArrayAccess)]
That would be a better UI! And if there's a way to find a ClassEntry
based on name, then that would solve it.
Is there a way currently to implement an interface defined in PHP code rather than built-in?
If we made this change we'd need to worry about backwards compatibility though, if that's important.
I guess it's not possible to make the PHP context exist using
cargo php stubs
?
Not sure, but prob. not that easy. But I might look into this.
By saying "use the interface name" you mean this?
#[implements(ArrayAccess)]
Exactly
Is there a way currently to implement an interface defined in PHP code rather than built-in?
If I understand it correctly ClassEntry::try_find
would be what you would use? I'll try to dig through the zend code tomorrow to understand how to fetch the interfaces.
If we made this change we'd need to worry about backwards compatibility though, if that's important.
Ye, would be a breaking change. But as crate is 0.x.x
it should be possible. But that is something a maintainer would need to answer @ptondereau @danog
So if ClassEntry::try_find
indeed also finds PHP-defined interfaces, this would actually support a new use case too.
An argument for using "ce::foo()" is discoverability: you can find that the PHP interface exists in the API documentation. But you'd only do that to actually find how to support a PHP interface you already know the name of, so that doesn't really work. Plus since the "call" is inside an #[implements()]
macro, IDE support isn't going to pop up anyway. So I think using the PHP names would actually be cleaner overall.
Take everything I say here with a grain of salt. Not that well versed in php zend api. Just some thoughts after reading some of the src.
So if
ClassEntry::try_find
indeed also finds PHP-defined interfaces, this would actually support a new use case too.
This won't work as it is, because PHP interfaces are not available when the extension loads (or might never be available depending on what code you are running). Also not much of a benefit, as those are not 'Magic' interfaces.
Regarding how to find the class entry id need to do some more digging.
Maybe as a temp workaround just adding a second optional parameter to #[implements()]
for stub generation would be easiest.
Yeah, I realized that the PHP defined interfaces wouldn't be available too. I still think it would be useful: PHP ecosystems likely exist that define new interfaces outside of the core ones and it makes sense to me someone may want to implement them using Rust. But it's not a priority.
I thought of a second parameter as well. It would certainly be the easiest to implement. I wonder though that this may make the upgrade path harder should we ever want to go name-only.
An alternative would be to implement the full new system and use a new directive like #[interface()]
to declare them.
Or to have a second parameter to trigger the new interpretation of the first, then issue warnings if it is false, qnd eventually make it superfluous, should we care about a migration path.
The docs here:
https://davidcole1340.github.io/ext-php-rs/macros/classes.html#implementing-an-interface
tell me I can use
#[implements()]
to implement an interface.And indeed using this test code:
array access and use of the
count
function both work as I expect when I execute things:But my editor tooling (PHP Intelephense in vs code) claims I can't use the
count
function as "Expected type 'Countable|array'. Found 'EvenNumbersArray'.".Looking at the generated PHP stubs, something seems to be wrong with the interface declaration - it's as if the Rust strings are directly inserted:
and at least Intelephense says that
::
is a syntax error.I think it should be:
Strangely enough I don't get a complaint about array accesses here, even though the proper array access isn't declared.
PS. In non-example code I also am having trouble convincing not only stubs but also the PHP runtime that something is an array or countable, but that does work in this minimal example so I'm still looking at what could have caused this.