Everything

php

ze1_compatibility_mode and cloning

php.internals

Rob Richards

22 years ago

ze1_compatibility_mode when set on calls clone on objects implicitly and is
causing some issues with extensions such as dom and xsl to name a few. Is it
possible to add something like the contained patch, which would allow an
object to implement an additional clone handler used only when the clone
call is from the engine's ze1_compatibility_mode calls?

i.e. the objects ze1 clone handler would be something like:
zend_object_value dom_objects_ze1_clone_obj(zval *zobject TSRMLS_DC)
{
zend_objects_store_add_ref(zobject TSRMLS_CC);

return zobject->value.obj;
}

so that a reference is returned rather than really cloning the internal
object as a real clone is not desireable here unless the developer
explicitly calls clone $obj.

Rob

Andi Gutmans

22 years ago

What is the issue? In general, as it's compatibility mode it's not supposed
to solve 100% but only 99.5% of the problems, and I don't really want to
change the engine's general API unless it's for a good reason.
Actually I have not seen many applications that have been broken due to the
removal of auto-cloning in PHP 5.
I think that as porting is required to use the new DOM/XSL extensions
anyway, they should also port their OO code. The idea was that old
untouched code would run, but if you want to take advantage of new features
you're best to port all the way.

Andi

At 04:50 PM 8/26/2004 -0400, Rob Richards wrote:

Rob Richards

22 years ago

The issue is that with some of the new extensions, the object "breaks" when
compatibility mode is enabled making the extensions useless. The patch was
basically an attempt to allow a way for the extensions to be able to run
under compatibility mode.
Take for example a simple case (NULL is returned with var_dump under compat
mode):
<?php
$dom = new DomDocument();
$dom->loadXML("<root> hello </root>");
$element = $dom->documentElement;
var_dump($element->parentNode);
?>

This is due to the clone functionality, which is correct when someone does
"clone $obj". Either there should be a way to allow extensions to be written
to support compat mode and clone (providing different functionality), compat
mode should be able to be disabled in an extension while clone syntax
supported, or a decision should be made that in cases where the two cant
have the same functionality that clone just shouldnt be supported.

Im not sure about other new extensions running into this, but simplexml is
another I played around with that runs into the same thing.

Rob

From: Andi Gutmans

> What is the issue? In general, as it's compatibility mode it's not

supposed

> to solve 100% but only 99.5% of the problems, and I don't really want to
> change the engine's general API unless it's for a good reason.
> Actually I have not seen many applications that have been broken due to

the

> removal of auto-cloning in PHP 5.
> I think that as porting is required to use the new DOM/XSL extensions
> anyway, they should also port their OO code. The idea was that old
> untouched code would run, but if you want to take advantage of new

features

Andi Gutmans

22 years ago

Hi Rob,

I understand the problem although I haven't analyzed it in as much depth as
you have as to when exactly it happens.
The thing is that I see compatibility_mode as something which is supposed
to help make most old scripts run with PHP 5. I think scripts that take
advantage of new PHP 5 features such as SimpleXML and friends should be
required to not run in compatibility_mode.
No I know DOM existed in PHP 4, but it has changed and I think it's fair to
require that if a person ports their application to PHP 5 DOM then they
port the whole thing.
You know I'm probably one of the strongest BC advocated on this list but I
think it's best not to change this.
Do you understand where I'm coming from?
Thanks,
Andi

At 09:07 PM 8/26/2004 -0400, Rob Richards wrote:

Rob Richards

22 years ago

That would be fine to not allow the problem extensions to run under ze1 and
end up issuing zend_error(E_ERROR, "Trying to clone an uncloneable object of
class %s",  Z_OBJCE_P(value)->name) (which currently happens when no clone
handler is implimented).

In order for that to happen though the extensions cannot support the new
clone syntax either since they use the same handler. If there arent going to
be any changes to the way this works, then I would like to make a BC break
in these extensions right now while its still early and remove their clone
functionailty (so far it would be dom, xsl and simplexml as I have only been
testing this with the xml extensions).

Rob

From: Andi Gutmans

> I understand the problem although I haven't analyzed it in as much depth

as

> you have as to when exactly it happens.
> The thing is that I see compatibility_mode as something which is supposed
> to help make most old scripts run with PHP 5. I think scripts that take
> advantage of new PHP 5 features such as SimpleXML and friends should be
> required to not run in compatibility_mode.
> No I know DOM existed in PHP 4, but it has changed and I think it's fair

to

Andi Gutmans

22 years ago

At 05:54 AM 8/27/2004 -0400, Rob Richards wrote:

>That would be fine to not allow the problem extensions to run under ze1 and
>end up issuing zend_error(E_ERROR, "Trying to clone an uncloneable object of
>class %s",  Z_OBJCE_P(value)->name) (which currently happens when no clone
>handler is implimented).
>
>In order for that to happen though the extensions cannot support the new
>clone syntax either since they use the same handler. If there arent going to
>be any changes to the way this works, then I would like to make a BC break
>in these extensions right now while its still early and remove their clone
>functionailty (so far it would be dom, xsl and simplexml as I have only been
>testing this with the xml extensions).

I don't see why you should remove the clone functionality of these
extensions if they work with standard PHP 5? The fact that it doesn't work
with compatibility mode isn't a good enough reason in my opinion. That
said, what you could do is have two sets of handlers arrays, one with the
clone and one with NULL. You can then check ze1.compatibilit_mode in the
extension and use the relevant handlers array.
What do you think?
Andi

Rob Richards

22 years ago

I hadn't even thought about that but it should give the desired results.
Can the error message be modified slightly to indicate that it is running
under ze1 compat in zend_execute.c?
Right now it just says: "Trying to clone an uncloneable object of class %s",
which is fine in most cases, but at least indicating that the clone was an
attempt due to ze1 compat would be more helpful and hopefully reduce bug
reports.

If all works well, is it ok to make this change in the 5_0 branch as well as
this will be a minor BC break and rather do this sooner than later (and I
mean minor as they dont work correctly anyways under compat mode), but now
will produce E_ERROR.

The following ext are the ones I would like to change:
dom
simplexml (right now it works fine as long as it is read only, but once
someone tries to change the xml, it doesn't work correctly)
xsl - remove clone support completely as there is no copy function for an
xslt stylesheet anyways

Rob

From: Andi Gutmans

Andi Gutmans

22 years ago

At 01:56 PM 8/27/2004 -0400, Rob Richards wrote:

>I hadn't even thought about that but it should give the desired results.
>Can the error message be modified slightly to indicate that it is running
>under ze1 compat in zend_execute.c?
>Right now it just says: "Trying to clone an uncloneable object of class %s",
>which is fine in most cases, but at least indicating that the clone was an
>attempt due to ze1 compat would be more helpful and hopefully reduce bug
>reports.

Well again, I'd like to keep the core part without too many branches. So if
you want to change the message, then best to implement the clone handler
for ze1.compatibility_mode and throw an E_ERROR there with the message you
think is suitable.

>If all works well, is it ok to make this change in the 5_0 branch as well as
>this will be a minor BC break and rather do this sooner than later (and I
>mean minor as they dont work correctly anyways under compat mode), but now
>will produce E_ERROR.

I think it makes sense.

>The following ext are the ones I would like to change:
>dom
>simplexml (right now it works fine as long as it is read only, but once
>someone tries to change the xml, it doesn't work correctly)
>xsl - remove clone support completely as there is no copy function for an
>xslt stylesheet anyways

Andi