class_exist bool autoload should be false if __autoload function doesn't... #959

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@ellisium

class_exist:: bool autoload should be false if __autoload function doesn't exist
cf issue #947

class_exist bool autoload should be false if __autoload function does…
…n't exist

class_exist::  bool autoload should be false if __autoload function doesn't exist 
issue #947
@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 15, 2013

Contributor

Hi @ellisium, before we can look at this, we need you to fill out a Contributor License Agreement as it doesn't appear you've ever contributed to a Facebook OSS project before:

https://developers.facebook.com/opensource/cla

Please let us know when you've done so and we'll review this. Thanks!

Contributor

scannell commented Aug 15, 2013

Hi @ellisium, before we can look at this, we need you to fill out a Contributor License Agreement as it doesn't appear you've ever contributed to a Facebook OSS project before:

https://developers.facebook.com/opensource/cla

Please let us know when you've done so and we'll review this. Thanks!

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 15, 2013

hi @scannell, that's signed

hi @scannell, that's signed

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 15, 2013

Can you add a positive and a negative test case for this to the PR?

Can you add a positive and a negative test case for this to the PR?

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 15, 2013

Contributor

Thanks for filling that out. See my comment on the commit itself -- tests are needed to verify this fixes the issue.

Contributor

scannell commented Aug 15, 2013

Thanks for filling that out. See my comment on the commit itself -- tests are needed to verify this fixes the issue.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 15, 2013

Could you explain me more what you want and where?

Could you explain me more what you want and where?

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 15, 2013

Contributor

Sure. In hphp/test is all of our test cases. You should probably add a new one in test/slow/ext_class to verify this change fixes the issue discussed in #947. See #955 for a pull request that has a bunch of tests. You can use the hphp/test/run script to run all of the tests, see the help for that.

Contributor

scannell commented Aug 15, 2013

Sure. In hphp/test is all of our test cases. You should probably add a new one in test/slow/ext_class to verify this change fixes the issue discussed in #947. See #955 for a pull request that has a bunch of tests. You can use the hphp/test/run script to run all of the tests, see the help for that.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 16, 2013

hi, sorry I can't reproduce the stackoverflow error with a simple test case.
But I confirm it fix #947
You can easily check it trying drupal install (in my case version 7.22)
About me, that's finished, I work on next issue

hi, sorry I can't reproduce the stackoverflow error with a simple test case.
But I confirm it fix #947
You can easily check it trying drupal install (in my case version 7.22)
About me, that's finished, I work on next issue

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 16, 2013

Contributor

Hi @ellisium, we're not going to take this as-is (no tests, no verification this doesn't otherwise break spl_autoload and other named functions) so closing this PR. If someone wants to take another look at this, feel free.

Contributor

scannell commented Aug 16, 2013

Hi @ellisium, we're not going to take this as-is (no tests, no verification this doesn't otherwise break spl_autoload and other named functions) so closing this PR. If someone wants to take another look at this, feel free.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 19, 2013

hi @scannell, finally I've worked on test case this weekend, and got it. Is it possible to reopen this PR or I've to do a new one? and how add the test case?

hi @scannell, finally I've worked on test case this weekend, and got it. Is it possible to reopen this PR or I've to do a new one? and how add the test case?

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 19, 2013

Contributor

Hi Eric -

I fixed the reason you'd see this with Drupal (should be pushed shortly) by reporting that we implement filter and SPL so this should be lower priority for now.

We won't take this version of the fix because it puts an expensive function_exists check on a very common path which is unnecessary the vast majority of the time and will affect overall performance of HHVM. If you want to pursue this, from an internal code review discussion of this:

First, the check should go on the slow path. In other words, wait until we decide we actually need to autoload before doing any special-case logic.

Second, you need to figure out what zend does so we can mirror autoload functionality accurately. Does it disable all autoloading from within any autoload handler? Does it just disable autoloading for any class which is actively being autoloaded? Can you spl_autoload a class which is already being __autoloaded? I'm also assuming it has nothing to do with class_exists, but rather autoloading in general; but you should check that too.

Contributor

scannell commented Aug 19, 2013

Hi Eric -

I fixed the reason you'd see this with Drupal (should be pushed shortly) by reporting that we implement filter and SPL so this should be lower priority for now.

We won't take this version of the fix because it puts an expensive function_exists check on a very common path which is unnecessary the vast majority of the time and will affect overall performance of HHVM. If you want to pursue this, from an internal code review discussion of this:

First, the check should go on the slow path. In other words, wait until we decide we actually need to autoload before doing any special-case logic.

Second, you need to figure out what zend does so we can mirror autoload functionality accurately. Does it disable all autoloading from within any autoload handler? Does it just disable autoloading for any class which is actively being autoloaded? Can you spl_autoload a class which is already being __autoloaded? I'm also assuming it has nothing to do with class_exists, but rather autoloading in general; but you should check that too.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 19, 2013

ok if that's affecting performance of HHVM, it's logical to search another way.
thx

ok if that's affecting performance of HHVM, it's logical to search another way.
thx

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 20, 2013

@scannell, I've traced the actual process with coding error omitting autoload=false
class_exists ->
getClass->
loadMissingClass (shouldn't be called because autoload should be false) ->
AutoloadHandler::s_instance->invokeHandler in which we find the same expensive function_exists(s___autoload)

if (m_handlers.isNull() && !forceSplStack) {
if (function_exists(s___autoload)) {
invoke(s___autoload, params, -1, true, false);
m_running = l_running;
return true;
}
m_running = l_running;
return false;
}

autoloadHandler return false so the function loadMissingClass falls in loop and produce finally the stackoverflow.

my patch use expensive function_exists(s___autoload), it's true but autoloader use it also and anyway autoloader should not be called in this case, I think to call autoloader is more expensive? no?

For me, it's really a problem about class_exists and not like you say about autoloader in general.

@scannell, I've traced the actual process with coding error omitting autoload=false
class_exists ->
getClass->
loadMissingClass (shouldn't be called because autoload should be false) ->
AutoloadHandler::s_instance->invokeHandler in which we find the same expensive function_exists(s___autoload)

if (m_handlers.isNull() && !forceSplStack) {
if (function_exists(s___autoload)) {
invoke(s___autoload, params, -1, true, false);
m_running = l_running;
return true;
}
m_running = l_running;
return false;
}

autoloadHandler return false so the function loadMissingClass falls in loop and produce finally the stackoverflow.

my patch use expensive function_exists(s___autoload), it's true but autoloader use it also and anyway autoloader should not be called in this case, I think to call autoloader is more expensive? no?

For me, it's really a problem about class_exists and not like you say about autoloader in general.

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 20, 2013

Contributor

It's fine for this particular case to be expensive. (If we really are going to invoke the autoloader, it's going to be expensive anyway, you are definitely correct.)

The problem is that Unit::classExists is called on multiple codepaths and adding the call to function_exists (which is not a trivial operation if you look it what it eventually does) slows all of them down.

To put it another way, in a PHP application where there is never any autoloaded code and no autoload handler is ever registered (__autoload, spl_autoload_register, etc.), this means every time class existence is checked, function_exists will be called.

Contributor

scannell commented Aug 20, 2013

It's fine for this particular case to be expensive. (If we really are going to invoke the autoloader, it's going to be expensive anyway, you are definitely correct.)

The problem is that Unit::classExists is called on multiple codepaths and adding the call to function_exists (which is not a trivial operation if you look it what it eventually does) slows all of them down.

To put it another way, in a PHP application where there is never any autoloaded code and no autoload handler is ever registered (__autoload, spl_autoload_register, etc.), this means every time class existence is checked, function_exists will be called.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 20, 2013

effectively, I'm completely agree and anyway test function_exists(s___autoload) should be in getClass and not in classExists so this PR is wrong.
I've just tried to explain that's not autoloader problem.

in getClass could be :
if (tryAutoload && !function_exists(s___autoload)) {
return loadMissingClass(ne, name);
}
or maybe using AutoloadHandler::s_instance->isRunning() faster but not sure right way
if (tryAutoload && AutoloadHandler::s_instance->isRunning()) {
return loadMissingClass(ne, name);
}
that's the last step before to call autoloader::invokeHandler.
Can I use isRunning() in this case?
Thank you very much about your patient and sorry for my bad english

effectively, I'm completely agree and anyway test function_exists(s___autoload) should be in getClass and not in classExists so this PR is wrong.
I've just tried to explain that's not autoloader problem.

in getClass could be :
if (tryAutoload && !function_exists(s___autoload)) {
return loadMissingClass(ne, name);
}
or maybe using AutoloadHandler::s_instance->isRunning() faster but not sure right way
if (tryAutoload && AutoloadHandler::s_instance->isRunning()) {
return loadMissingClass(ne, name);
}
that's the last step before to call autoloader::invokeHandler.
Can I use isRunning() in this case?
Thank you very much about your patient and sorry for my bad english

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 20, 2013

Contributor

Does Zend allow __autoload for a different class to be called inside an spl_autoload_register handler? (It may not be correct to disallow autolading from inside an autoloader.)

Contributor

scannell commented Aug 20, 2013

Does Zend allow __autoload for a different class to be called inside an spl_autoload_register handler? (It may not be correct to disallow autolading from inside an autoloader.)

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 21, 2013

In my test case, Zend doesn't allow it. Server crash when calling a different class inside spl_autoload_register function

In my test case, Zend doesn't allow it. Server crash when calling a different class inside spl_autoload_register function

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 22, 2013

Contributor

Where is the test case? It would be helpful to see what is going on here. You should also try just chaining __autoload together -- class A autoloads class B which has to autoload class C. That seems like something Zend should allow as long as A != B != C?

Contributor

scannell commented Aug 22, 2013

Where is the test case? It would be helpful to see what is going on here. You should also try just chaining __autoload together -- class A autoloads class B which has to autoload class C. That seems like something Zend should allow as long as A != B != C?

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 22, 2013

Contributor

Checking AutoloadHandler::s_instance->isRunning() probably isn't sufficient since I'm guessing the above (chaining autoload of classes) is allowed.

Also, come to think of it, is checking function_exists even enough here? The presence of __autoload doesn't mean we can't try autoloading, it just means that we can't recursively call an autoload handler.

Contributor

scannell commented Aug 22, 2013

Checking AutoloadHandler::s_instance->isRunning() probably isn't sufficient since I'm guessing the above (chaining autoload of classes) is allowed.

Also, come to think of it, is checking function_exists even enough here? The presence of __autoload doesn't mean we can't try autoloading, it just means that we can't recursively call an autoload handler.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 22, 2013

you can find tests case here https://github.com/ellisium/hiphop-php/tree/patch-1/hphp/test/slow/autoload

"Does Zend allow __autoload for a different class to be called inside an spl_autoload_register handler? (It may not be correct to disallow autolading from inside an autoloader.)"
finally yes and no (see a-in-b.php and a-in-b-crash.php)

"Where is the test case? It would be helpful to see what is going on here. You should also try just chaining __autoload together -- class A autoloads class B which has to autoload class C. That seems like something Zend should allow as long as A != B != C?"
seems not allowed, (see 1-2-3.php)

"@ellisium, see #976, more evidence this is really a broader problem with autoloading and not just with class_exists."
not class_exist problem. getClass problem, call getClass with autoload =true or omitted on class not loaded from autoload will produce stackoverflow also

I confirm AutoloadHandler::s_instance->isRunning() and function_exists, are not the right way. by example that break prestashop autoload

you can find tests case here https://github.com/ellisium/hiphop-php/tree/patch-1/hphp/test/slow/autoload

"Does Zend allow __autoload for a different class to be called inside an spl_autoload_register handler? (It may not be correct to disallow autolading from inside an autoloader.)"
finally yes and no (see a-in-b.php and a-in-b-crash.php)

"Where is the test case? It would be helpful to see what is going on here. You should also try just chaining __autoload together -- class A autoloads class B which has to autoload class C. That seems like something Zend should allow as long as A != B != C?"
seems not allowed, (see 1-2-3.php)

"@ellisium, see #976, more evidence this is really a broader problem with autoloading and not just with class_exists."
not class_exist problem. getClass problem, call getClass with autoload =true or omitted on class not loaded from autoload will produce stackoverflow also

I confirm AutoloadHandler::s_instance->isRunning() and function_exists, are not the right way. by example that break prestashop autoload

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 22, 2013

Contributor

Any ideas on what we need to do here then? spl_autoload_register can be used to register a bunch of autoload handlers in order. It seems like the a-b example does NOT recursively autoload. What happens to prestashop if you just use isRunning()? [Is the problem that it sets that value before getClass is called the first time to autoload?]

Also, I should add, I'm glad we're making progress here -- thanks for spending time on this.

Contributor

scannell commented Aug 22, 2013

Any ideas on what we need to do here then? spl_autoload_register can be used to register a bunch of autoload handlers in order. It seems like the a-b example does NOT recursively autoload. What happens to prestashop if you just use isRunning()? [Is the problem that it sets that value before getClass is called the first time to autoload?]

Also, I should add, I'm glad we're making progress here -- thanks for spending time on this.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 23, 2013

I report you my test of today
CASE native
getClass :
if (tryAutoload) {
return loadMissingClass(ne, name);
}
prestashop -> ok
drupal -> HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/drupal/includes/bootstrap.inc on line 3029
wordpress -> can't test HipHop Warning: Case insensitive constant names are not supported in HipHop in /home/ellisium/prog/hhvm/www/wordpress/wp-includes/wp-db.php on line 20
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

CASE AutoloadHandler::s_instance->isRunning()
getClass :
if (tryAutoload && AutoloadHandler::s_instance->isRunning()) {
return loadMissingClass(ne, name);
}
prestashop -> HipHop Fatal error: unknown class PrestaShopException in /home/ellisium/prog/hhvm/www/prestashop/install/classes/exception.php on line 27
drupal -> ok
wordpress -> ok (my own hhvm version)
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

CASE !function_exists(s__autoload)
getClass :
if (tryAutoload && !function_exists(s__autoload)) {
return loadMissingClass(ne, name);
}
prestashop -> ok
drupal -> ok
wordpress -> ok (my own hhvm version)
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

Do you want I open a new issue about 1-2-3.php test case?
Tomorrow I'll test my others CMS.

"Is the problem that it sets that value before getClass is called the first time to autoload?" Sorry, don't get it.

I report you my test of today
CASE native
getClass :
if (tryAutoload) {
return loadMissingClass(ne, name);
}
prestashop -> ok
drupal -> HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/drupal/includes/bootstrap.inc on line 3029
wordpress -> can't test HipHop Warning: Case insensitive constant names are not supported in HipHop in /home/ellisium/prog/hhvm/www/wordpress/wp-includes/wp-db.php on line 20
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

CASE AutoloadHandler::s_instance->isRunning()
getClass :
if (tryAutoload && AutoloadHandler::s_instance->isRunning()) {
return loadMissingClass(ne, name);
}
prestashop -> HipHop Fatal error: unknown class PrestaShopException in /home/ellisium/prog/hhvm/www/prestashop/install/classes/exception.php on line 27
drupal -> ok
wordpress -> ok (my own hhvm version)
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

CASE !function_exists(s__autoload)
getClass :
if (tryAutoload && !function_exists(s__autoload)) {
return loadMissingClass(ne, name);
}
prestashop -> ok
drupal -> ok
wordpress -> ok (my own hhvm version)
joomla ->
HipHop Warning: Invalid argument: function: method '_autoload' not found in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
HipHop Fatal error: Class undefined: JVersion in /home/ellisium/prog/hhvm/www/joomla/libraries/cms.php on line 38
a-b -> ok
b-a -> ok
a-in-b -> ok
a-in-b-crash -> ok (crash php also) HipHop Fatal error: Stack overflow in /home/ellisium/prog/hhvm/www/test1/a-in-b-crash.php on line 3
b-in-a -> ok
1-2-3 -> HipHop Fatal error: Function already defined: __autoload in /home/ellisium/prog/hhvm/www/test1/class_1.php on line 7

Do you want I open a new issue about 1-2-3.php test case?
Tomorrow I'll test my others CMS.

"Is the problem that it sets that value before getClass is called the first time to autoload?" Sorry, don't get it.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 24, 2013

Today, I've investigated on joomla problem.
That's a new issue about spl_autoload_register. Normally, when spl_autoload_register inside a class, you can call the function_autoload in class no matter if function is private or public. HHVM doesn't allow to use private function.
I've created test case:
spl_autoload_register_class_private.php
spl_autoload_register_class_public.php.

So about joomla:
CASE Native -> ok
CASE AutoloadHandler::s_instance->isRunning() -> no erros but white screen
CASE !function_exists(s__autoload) -> ok

Today, I've investigated on joomla problem.
That's a new issue about spl_autoload_register. Normally, when spl_autoload_register inside a class, you can call the function_autoload in class no matter if function is private or public. HHVM doesn't allow to use private function.
I've created test case:
spl_autoload_register_class_private.php
spl_autoload_register_class_public.php.

So about joomla:
CASE Native -> ok
CASE AutoloadHandler::s_instance->isRunning() -> no erros but white screen
CASE !function_exists(s__autoload) -> ok

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 26, 2013

Contributor

Joomla classes are defining __autoload as private?

Contributor

scannell commented Aug 26, 2013

Joomla classes are defining __autoload as private?

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 27, 2013

Example is better than words

that was a big surprise to discover that ZEND allow spl_autoload_register to access a private or protected function loader.
With ZEND we can autoload like that but not with HHVM (even if it's more logical).
So inside lookupMethodCtx in bytecode.cpp, we have to override private or protected property for to return the method. after check the class and method are from spl_autoload_register.

I've fixed it, but I would like to write it in better way (using class) but for that I need to update spl extension and command "/prog/hhvm/hiphop-php/hphp$ EXT=SPL make -C idl update" doesn't work
Please, Could you indicate me the right way ? thx

Example is better than words

that was a big surprise to discover that ZEND allow spl_autoload_register to access a private or protected function loader.
With ZEND we can autoload like that but not with HHVM (even if it's more logical).
So inside lookupMethodCtx in bytecode.cpp, we have to override private or protected property for to return the method. after check the class and method are from spl_autoload_register.

I've fixed it, but I would like to write it in better way (using class) but for that I need to update spl extension and command "/prog/hhvm/hiphop-php/hphp$ EXT=SPL make -C idl update" doesn't work
Please, Could you indicate me the right way ? thx

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 27, 2013

Contributor

You can edit the ext_spl.h by hand. (We generally only use that script for initial generation, the update hasn't worked in a while; sorry about that.)

Contributor

scannell commented Aug 27, 2013

You can edit the ext_spl.h by hand. (We generally only use that script for initial generation, the update hasn't worked in a while; sorry about that.)

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 27, 2013

Hi, thx for this fast answer.
Before to try update command I've tried by hand but doesn't compile.

class SPL : public Registered {
public:
bool bool_spl_register_class(String spl_class);
void set_spl_register_class(String spl_class);
void unset_spl_register_class(String spl_class);
private:
Array spl_registered;
}

In file included from /home/ellisium/prog/hhvm/hiphop-php/hphp/runtime/ext/ext.h:80:0,
from /home/ellisium/prog/hhvm/hiphop-php/hphp/system/class_map.cpp:3:
/home/ellisium/prog/hhvm/hiphop-php/hphp/runtime/ext/ext_spl.h:27:41: erreur: expected class-name before ‘{’ token
Don't know what's wrong with this short code...

Hi, thx for this fast answer.
Before to try update command I've tried by hand but doesn't compile.

class SPL : public Registered {
public:
bool bool_spl_register_class(String spl_class);
void set_spl_register_class(String spl_class);
void unset_spl_register_class(String spl_class);
private:
Array spl_registered;
}

In file included from /home/ellisium/prog/hhvm/hiphop-php/hphp/runtime/ext/ext.h:80:0,
from /home/ellisium/prog/hhvm/hiphop-php/hphp/system/class_map.cpp:3:
/home/ellisium/prog/hhvm/hiphop-php/hphp/runtime/ext/ext_spl.h:27:41: erreur: expected class-name before ‘{’ token
Don't know what's wrong with this short code...

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 27, 2013

Contributor

Two things:

  1. Not sure where Registered is defined, it's not one of our classes.
  2. You need a semicolon after the class definition.
Contributor

scannell commented Aug 27, 2013

Two things:

  1. Not sure where Registered is defined, it's not one of our classes.
  2. You need a semicolon after the class definition.
@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 28, 2013

thx, it's ok about class now.
But I've a problem with an Array "spl_registered" which store String

class SPL
{
public:
static SPL& GetInstance()
{
static SPL c_spl;
return c_spl;
}
bool bool_spl_register_class(CStrRef spl_class);
void set_spl_register_class(CStrRef spl_class);
void unset_spl_register_class(CStrRef spl_class);
private:
SPL() {}
SPL(const SPL&);
SPL& operator=(const SPL&);
Array spl_registered;
};

console error:
load page works as expected
ArrayData(1): array(1) {
[0]=>
string(23) "ClassAutoloader::loader"
}
Reload page :
ArrayData(1): array(1) {
[0]=>
string(23) "ClassAutoloa���v���"
}
Reload page :
Erreur de segmentation (core dumped)

thx, it's ok about class now.
But I've a problem with an Array "spl_registered" which store String

class SPL
{
public:
static SPL& GetInstance()
{
static SPL c_spl;
return c_spl;
}
bool bool_spl_register_class(CStrRef spl_class);
void set_spl_register_class(CStrRef spl_class);
void unset_spl_register_class(CStrRef spl_class);
private:
SPL() {}
SPL(const SPL&);
SPL& operator=(const SPL&);
Array spl_registered;
};

console error:
load page works as expected
ArrayData(1): array(1) {
[0]=>
string(23) "ClassAutoloader::loader"
}
Reload page :
ArrayData(1): array(1) {
[0]=>
string(23) "ClassAutoloa���v���"
}
Reload page :
Erreur de segmentation (core dumped)

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Aug 28, 2013

Contributor

Not sure. I'd attach gdb and debug it since it's something at the lower level.

Contributor

scannell commented Aug 28, 2013

Not sure. I'd attach gdb and debug it since it's something at the lower level.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Aug 29, 2013

it's ok, sorry my fault. I thought all HPHP class (Array, String, etc) were reset after end of request but I was wrong so my code corrupted memory.
With RequestEventHandler, everything work as expected.
Edit :
a new issue found "can't call __autoload after spl_autoload_unregister"
(test case written spl_autoload_unregister.php)
Edit:
@scannell, should I create a PR for each issues menitonned here or a common PR is ok?

it's ok, sorry my fault. I thought all HPHP class (Array, String, etc) were reset after end of request but I was wrong so my code corrupted memory.
With RequestEventHandler, everything work as expected.
Edit :
a new issue found "can't call __autoload after spl_autoload_unregister"
(test case written spl_autoload_unregister.php)
Edit:
@scannell, should I create a PR for each issues menitonned here or a common PR is ok?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 1, 2013

"that was a big surprise to discover that ZEND allow spl_autoload_register to access a private or protected function loader.
With ZEND we can autoload like that but not with HHVM (even if it's more logical)."

You get unpredictable issues when you register a private autoloader like this.

The reason you CAN do it is because you are calling spl_autoload_register from within the class itself, ie:
static class ClassAutoloader {
public static function setup() {
spl_autoload_register(array('ClassAutoloader::loader'));
}
private static function loader($className) {
echo 'Trying to load ', $className, ' via ', _METHOD, "()\n";
require './class
'.str_replace("Core","",$className) . '.php';
}
}

Now with an example of using it:
$myclass = new CoreUnknownClass();

This will attempt to include the file ./class_UnknownClass.php
Everything here works just fine as long as your programmers don't make any typo's. But what if they saved the fille as ./class_UknownClass.php?

require fails and will cause the application to abort immediately. Much much much worse, because this occurred in a private method, you don't get ANY error information[all the require error messages are lost] No line numbers, no idea what the problem is.

Even XDebug doesn't help much. With XDebug you can get to the line $myclass=new CoreUnknownClass(); but it can't step into the autoloader since it is private, it will die on that line.

As such, even though Zend "allows" this to be done, it does not /support/ it being done - it is an accident - as such I really don't think you should copy this functionality - it's a Zend bug, not a feature.

If changing that one word from "private" to "public" will allow the Joomla unit tests to run, then the issue is a Joomla issue - not a hiphop issue.

ghost commented Oct 1, 2013

"that was a big surprise to discover that ZEND allow spl_autoload_register to access a private or protected function loader.
With ZEND we can autoload like that but not with HHVM (even if it's more logical)."

You get unpredictable issues when you register a private autoloader like this.

The reason you CAN do it is because you are calling spl_autoload_register from within the class itself, ie:
static class ClassAutoloader {
public static function setup() {
spl_autoload_register(array('ClassAutoloader::loader'));
}
private static function loader($className) {
echo 'Trying to load ', $className, ' via ', _METHOD, "()\n";
require './class
'.str_replace("Core","",$className) . '.php';
}
}

Now with an example of using it:
$myclass = new CoreUnknownClass();

This will attempt to include the file ./class_UnknownClass.php
Everything here works just fine as long as your programmers don't make any typo's. But what if they saved the fille as ./class_UknownClass.php?

require fails and will cause the application to abort immediately. Much much much worse, because this occurred in a private method, you don't get ANY error information[all the require error messages are lost] No line numbers, no idea what the problem is.

Even XDebug doesn't help much. With XDebug you can get to the line $myclass=new CoreUnknownClass(); but it can't step into the autoloader since it is private, it will die on that line.

As such, even though Zend "allows" this to be done, it does not /support/ it being done - it is an accident - as such I really don't think you should copy this functionality - it's a Zend bug, not a feature.

If changing that one word from "private" to "public" will allow the Joomla unit tests to run, then the issue is a Joomla issue - not a hiphop issue.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Oct 1, 2013

@garyamort in latest joomla version, load function has been changed to public.
I'm agree with you about, this one should be considerate as Zend bug.
However If u can't modify php code (for some reasons ex: license or host conditions where you can't know what kind of code your customer run). That's become a problem.
Anyway, scannell has written a PR for this issue

ellisium commented Oct 1, 2013

@garyamort in latest joomla version, load function has been changed to public.
I'm agree with you about, this one should be considerate as Zend bug.
However If u can't modify php code (for some reasons ex: license or host conditions where you can't know what kind of code your customer run). That's become a problem.
Anyway, scannell has written a PR for this issue

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Oct 1, 2013

Contributor

To be precise, the fix will treat the call to the autoloader as having been called from the context in which the loader was registered. In general, this is not recommended practice -- and I'd at least be happy to take this code out if the we can all agree that we should not do this and to deprecate the old behavior. PHP Internals is probably the right place for that discussion though.

Contributor

scannell commented Oct 1, 2013

To be precise, the fix will treat the call to the autoloader as having been called from the context in which the loader was registered. In general, this is not recommended practice -- and I'd at least be happy to take this code out if the we can all agree that we should not do this and to deprecate the old behavior. PHP Internals is probably the right place for that discussion though.

@ellisium

This comment has been minimized.

Show comment
Hide comment
@ellisium

ellisium Oct 1, 2013

@scannell, you're right, we are all agree for to say that access private/protected is wrong way. Before to out it, maybe zend developpers opinion is required. If they keep going in futur with this way, we can't skip it.
Just my own opinion

ellisium commented Oct 1, 2013

@scannell, you're right, we are all agree for to say that access private/protected is wrong way. Before to out it, maybe zend developpers opinion is required. If they keep going in futur with this way, we can't skip it.
Just my own opinion

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 2, 2013

@ellisium Not quite. In the latest Joomla framework there are 3 possible autoload methods in JLoader. 2 are public[load by namespace and load by psr0], load by prefix[the only method I know offhand, which is __autoload] is still private. Load by prefix is a legacy function that will be depreciated at some point, so I'll pull down the code and submit a pull request to make that public later this week.

I'm primarily a PHP programmer with a strong background in Joomla! so seeing it at 0% on the latest blog post piqued my interest. :-) My interest in HiphopPHP is actually to compare memory usage for running it on an embedded system[BeagleBoneBlack] since PHP is a rather hefty investment when you only have 512M of memory.

I just wanted to make sure you were aware that this functionality is not a Zend "feature" and that it is actually bad coding practice before spending a lot of time trying to replicate Zend's "bug" :-)

ghost commented Oct 2, 2013

@ellisium Not quite. In the latest Joomla framework there are 3 possible autoload methods in JLoader. 2 are public[load by namespace and load by psr0], load by prefix[the only method I know offhand, which is __autoload] is still private. Load by prefix is a legacy function that will be depreciated at some point, so I'll pull down the code and submit a pull request to make that public later this week.

I'm primarily a PHP programmer with a strong background in Joomla! so seeing it at 0% on the latest blog post piqued my interest. :-) My interest in HiphopPHP is actually to compare memory usage for running it on an embedded system[BeagleBoneBlack] since PHP is a rather hefty investment when you only have 512M of memory.

I just wanted to make sure you were aware that this functionality is not a Zend "feature" and that it is actually bad coding practice before spending a lot of time trying to replicate Zend's "bug" :-)

@scannell

This comment has been minimized.

Show comment
Hide comment
@scannell

scannell Oct 2, 2013

Contributor

@garyamort, thanks.

The effort has already been put in to fix this -- this falls under the case where enough widely-used existing PHP code uses it that we'll have to hold our nose since it's at least somewhat defensible (even if not the best coding practice.)

Re: embedded, I believe you will OOM with only 512 MB by default because of the slabs we reserve on startup, although that is configurable. That aside, we've focused our optimization efforts on datacenter-grade servers and definitely (within reason) prioritize reducing CPU usage over reducing RAM usage due to the cost factors involved. This is not to say we wouldn't also welcome contributions to reduce memory usage but it hasn't been an active area of investigation.

Contributor

scannell commented Oct 2, 2013

@garyamort, thanks.

The effort has already been put in to fix this -- this falls under the case where enough widely-used existing PHP code uses it that we'll have to hold our nose since it's at least somewhat defensible (even if not the best coding practice.)

Re: embedded, I believe you will OOM with only 512 MB by default because of the slabs we reserve on startup, although that is configurable. That aside, we've focused our optimization efforts on datacenter-grade servers and definitely (within reason) prioritize reducing CPU usage over reducing RAM usage due to the cost factors involved. This is not to say we wouldn't also welcome contributions to reduce memory usage but it hasn't been an active area of investigation.

@photodude photodude referenced this pull request in joomla/joomla-cms Oct 19, 2016

Closed

HHVM errors from Travis ci #10220

@photodude photodude referenced this pull request in joomla/joomla-cms Oct 19, 2016

Merged

Mark _autoload function as public and not private. #12478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment