-
Notifications
You must be signed in to change notification settings - Fork 200
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
Proposal: remove ad-hoc sync of V8Js object? #72
Comments
I'm all for simplifying the now-redundant code e.g., all of v8js_variables.cc now. Also I think that following the nodeJs patterns more closely makes sense too. @stesie — What are your thoughts? |
I also appreciate getting rid of the duplicate code and first time around I found it rather confusing. But I'm not sure whether it's a good idea to have a seperate object with the properties to expose around, since it would mean backwards incompatibility for no extra features. Besides I especially like to derive from the V8Js object, attaching properties to export from __construct, providing extra methods and then just instanciate the derived class -- which wouldn't be possible with the "exports object" approach (at least not as simple). On the other hand I already thought of flagging methods as "not to export" in some way, even so they are public. That is I've got a class that I'd like to export, but it e.g. has a public "injectData" or "loadData" method I don't want to see called from JS (or certain setters, whatever). Current approach is to wrap the object with a facade object which provides only the exportable methods, but that's tedious. That said I'm yet unsure how that could/should be declared. Either through docblock annotations (probably tricky to access, never tried; declaration written down close to the function however) or method calls to the V8Js object, telling never export method foo of class bar (however these probably will accumulate next to where V8Js is instanciated, so it isn't clear when looking at the class). This "not to export" functionality could then be used to hide "executeString" function, etc. |
Instead of using special JavaScript accessors for the `PHP` object, we just reuse the standard PHP object wrapping. To control which properties are visible to JavaScript, we use a separate PHP V8UserProperties object for the JavaScript-visible stuff. We need to use __get/__set/etc magic methods (instead of the object handler equivalents) in order to make things work properly with derived classes.
…port. There is a minor change to how properties of V8Js subclasses are exposed to JavaScript: subclass properties are *not* visible to JS by default. We preserve the read-only status of PHP objects exposed via V8Js properties by adding a hidden read-only flag to the V8 wrappers. PHP objects exposed to JS by other means (for example, return values of method invocations) are not read-only.
There is a minor change to how properties of V8Js subclasses are exposed to JavaScript: subclass properties are *not* visible to JS by default. More significantly, all values exposed to JavaScript are fully mutable.
I've pushed a set of patches to discuss/consider on the cscott/v8js#issue-72a (7090291) and cscott/v8js#issue-72b (2cedd40) branches. In the main commit (81d4418) you can see how much code simplification we're talking about here. The backwards incompatibilities can be seen by studying 7090291 on the issue-72a branch. Basically, the way derived classes work is slightly different -- any properties declared explicitly on the subclass are not exported to JS. This is reasonable, I think. It would be possible to add a The main difference here is that php objects exported via properties are no longer read-only. That might be a feature, that might be a bug. Commit ec8b16f on the issue-72b branch addresses this by adding a special "read only" flag on the V8 wrappers of PHP objects. Currently objects exported via properties on While I'm at it, I'll mention one other possible tweak here: right now the Thoughts? |
So I'm leaning towards making an explicit V8UserProperties object that proxies by default to properties on the V8Js object (instead of the above patches, which have V8Js proxying to V8UserProperties). This might let me settle another lingering issue -- I'd like to suppress the ability for user code to directly invoke |
@cscott are you still willing to work on that topic? I prefer the solution you suggested in your last post, that the V8UserProperties object proxies the properties on the V8Js object (and gets a reference via __construct); in combination with the user properties object being swappable this would be great. I also like the idea to be able to modify Hence I would
cheers |
@stesie I'm rather overcommitted at the moment, so feel free to steal this issue from me. I agree with your assessment, and that means basically throwing out my patches above and starting from scratch, so go ahead! You've got my blessing, for what it's worth. ;) I'd suggest that the exported For access control, users could subclass the V8Js object and override the Editing the |
FYI I've started working on that, see https://github.com/stesie/v8js/commits/issue-72 if you're interested |
The V8Js object is treated as a special case, and has its own handlers both on the V8 and the PHP sides. Now that V8 wrapping of PHP objects works much better (since f6a6d1e) I think it makes sense to remove a bunch of this code and just expose the V8Js object using the standard
zval_to_v8js
wrappers.However, there are some methods of the V8Js object which we don't want to directly expose to JS (like the
executeString
method), so I'm actually proposing that the V8Js object reference a separate "blank" object for user properties. Adding/removing/querying properties from the PHP side would be proxied over to the user properties object. The V8 wrapper for V8Js would be the standard zval_to_v8js of the user properies object.I think that would end up removing a bunch of now-redundant code (like all of
v8js_variables.cc
) and simplifying the codebase (likephp_v8js_write_property
etc).The text was updated successfully, but these errors were encountered: