Proposal: remove ad-hoc sync of V8Js object?

[cscott]

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 (like php_v8js_write_property etc).

[yllierop]

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?

[stesie]

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.

[cscott]

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 V8Js::getUserProperties() method to directly access the backing user properties object, so that $v8->foo='bar'; is more accurately seen as a helpful shortcut for $v8->getUserProperties()->foo = 'bar';. You could even provide a V8Js::setUserProperties($obj) method, since there is no "magic" involved in the user property wrapper anymore.

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 V8Js have the read-only flag set (and it's a deep flag, so it affects any properties of those objects, etc). This maintains the current read-only behavior (and thus fewer test results change, see 2cedd40) but it feels a little bit inflexible to me. Probably we'd need an API to better handle how PHP objects are "frozen". I'm also not 100% certain the read-only property is bulletproof; it's likely there are other ways to mutate objects (like by calling methods) and so I feel that this mechanism gives a false sense of security.

While I'm at it, I'll mention one other possible tweak here: right now the V8UserProperties object is a plain PHP object with no special wrapping or methods -- which would let you easily swap it out for a custom properties object if you liked. An alternative would be to put all the magic in the V8UserProperties object: it could have a private property in the constructor that determined whether would use read-only wrappers or not, and the default implementation would have special __get/__set/etc magic methods that would expose the public properties of the attached V8Js object. If you wanted more fine-grained control, you could again swap out the V8UserProperties object for a different one. In fact, probably the third argument to the V8Js constructor would be the properties object you wanted to use; but if you passed in a plain associative array (rather than a subclass of V8UserProperties) the $v8->foo shortcut might not work. (You could probably reimplement the shortcut in userspace with a __get magic method, though.)

Thoughts?

[cscott]

(Oh, and ignore the failing test results on the issue-72a and issue-72b branches; they depend on #76 and #77 in order to pass. I can rebase the branches once those are merged.)

[cscott]

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 print, var_dump, and sleep. Although there are hacky ways to do that, it might be cleaner to implementt v8js_methods.cc as 'native' methods on the V8UserProperties object. Then, again, you could pass in a custom subclass if you wanted to disable methods or change how the native print/sleep/exit/var_dump/require methods worked. And, again, we'd be reusing the standard v8->PHP method invocation code, instead of writing special cases. (It would be even nicer if I could code the default V8UserProperties object directly in PHP, instead of writing it in low-level zend. Do either of you know of a good way to bundle PHP source files with a zend extension?)

[stesie]

@cscott are you still willing to work on that topic?
I still like the idea and otherwise would step up and finish it.

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 print, var_dump et al, but I see one problem: we currently export the user properties under a object named "PHP" by default, methods are exported as global objects however. So we need some "legacy glue" that makes certain user properties available globally -- I'm leaning for a php.ini entry, enumerating method names, then an user would also be capable of adding extra methods and exporting those globally too.
To allow easy reuse of our default user functions in combination with custom user properties objects I'd like to have these within a trait and ditch PHP 5.3 compatibility.

Hence I would

• rebase first
• provide a V8UserFunctions trait
• implement a V8UserPropertiesProxy class that
  • ... uses the V8UserFunctions trait
  • ... has a __construct method to accept a V8Js instance, keeping a reference privatly
  • ... has __get, __set, __isset implementatitons, proxying to V8Js properties
  • ... has __call method that dispatches method calls normally, but throws if the method exists on V8Js object (this is methods on V8UserPropertiesProxy & classes derived from V8Js should still be callable; but V8Js->executeString et al shouldn't)
• any object passed to V8Js::__construct as argument 2 (currently array $variables) should be used instead of a default V8UserProperties instance
  • if $variables isn't passed, then create a default V8UserPropertiesProxy instance and inject V8Js object into it
  • if an array is passed as $variables then create the proxy object likewise, but attach properties on it
• there should be getUserProperties, setUserProperties methods on V8Js class
• add a php.ini entry v8js.global_user_functions defaulting to "print,sleep,exit,var_dump,require", which controlls which methods from the user properties object should be available globally (as well)
• add a php.ini entry v8js.legacy_user_properties_access defaulting to "false", which controls whether V8UserPropertiesProxy class should map in a read-write or read-only fashion
  • defaulting to "false" so the behaviour is more consistent eventually, but users can set it "true" if it breaks something on their side
  • I don't think we should have (or would need) an option to export other (user property) implementations read-only likewise, since the behaviour can easily be achieved using facade classes or own __get/__set implementations

cheers
~stesie

[cscott]

@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 print/sleep/exit/etc global functions are just wrappers that redirect to PHP.print/PHP.sleep/PHP.exit -- these can be written in JS entirely on the v8 side (ie, something like print = PHP.print.bind(PHP);). Thus users can do delete print; or set print = function() { /* my own print */ } from within their javascript startup code to delete/modify the global bindings, they wouldn't necessarily have to edit the php.ini.

For access control, users could subclass the V8Js object and override the print/sleep/etc method if they wanted to disable access or provide different bindings on the PHP side. (For example, they could provide their own alternate implementation of require, or allow exit only if the current user is authenticated, etc.)

Editing the php.ini to remove the automatically generated redirection wrappers on the JS side, or else disable the default V8Js methods on the PHP side would still be a useful feature to have, and I like the idea of "secure by default" settings. Making sure that the automatically exported bindings redirected to the single PHP.<method> entry point would allow fine-grained control even with "insecure"/legacy settings in php.ini.

[stesie]

FYI I've started working on that, see https://github.com/stesie/v8js/commits/issue-72 if you're interested