Details

Reviewers

Group Reviewers

Maniphest Tasks

T1116: Function __phutil_autoload should probably not throw an exception if it is the only registered autoload function

Commits

rPHU33b1620656f7: Revert "Don't set `YYINITDEPTH`"
rPHUe65ffd33db02: Don't throw `PhutilMissingSymbolException` from `class_exists` or…

Summary

Fixes T1116. Currently we throw a PhutilMissingSymbolException from the libphutil autoloader function (__phutil_autoload) if the following criteria are met:

The class/interface being loaded does not exist.
__phutil_autoload is the last registered autoloader.

This causes problems with some third-party code which does class_exists($some_class) where $some_class doesn't exist. This function causes the autoloader to be invoked, which thus throws PhutilMissingSymbolException. This is clearly undesirable and, as such, a rather hacky fix is to inspect the output of debug_backtrace() and look for a call to class_exists, interface_exists or trait_exists anywhere in the stacktrace. If any of these functions are found in the stacktrace, we simply return false instead of throwing a PhutilMissingSymbolException.

Test Plan

Ran ./scripts_update_compat_info.php without error.

Diff Detail

Repository

rPHU libphutil

Branch

master

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 9126
Build 10765: Run Core Tests
Build 10764: arc lint + arc unit

Event Timeline

joshuaspence updated this revision to Diff 35126.Nov 19 2015, 7:27 PM

joshuaspence retitled this revision from to Add a dummy autoload function to work around T1116..

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

joshuaspence added a task: T1116: Function __phutil_autoload should probably not throw an exception if it is the only registered autoload function.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 19 2015, 7:27 PM

Herald added a subscriber: Korvin. · View Herald Transcript

joshuaspence retitled this revision from Add a dummy autoload function to work around T1116. to Add a dummy autoload function to work around autoloader issue.Nov 19 2015, 7:36 PM

joshuaspence updated this object.

joshuaspence edited edge metadata.

joshuaspence mentioned this in D14520: Update PHP compatibility info.Nov 19 2015, 7:57 PM

What do you think about this approach instead?

Before throwing a "not found" exception, check if class_exists() is on the callstack.
(If it is, check if any other autoloaders are registered? I think it's fine to just proceed even in a pure libphutil environment.)
If they are (even if we are the last autoloader), return false / no-op instead of throwing.

That is, here and in T1116 the specific problem behavior is that callers invoke class_exists('x') and expect it to return false. The behavior of php-parser, at least, is not really correct: it should pass false, I think, to disable autoloading, or just not do the check at all because the include has no side effects, I think. But we're broadly powerless to change that since we can't send patches to every project ever, this usage is common, and some of them probably aren't strictly incorrect.

This revision now requires changes to proceed.Nov 23 2015, 3:28 PM

joshuaspence mentioned this in rPHU5538909d6468: Update PHP compatibility info.Nov 23 2015, 7:42 PM

epriestley mentioned this in T1116: Function __phutil_autoload should probably not throw an exception if it is the only registered autoload function.Nov 24 2015, 6:51 PM

Check for class_exists

joshuaspence retitled this revision from Add a dummy autoload function to work around autoloader issue to Don't throw `PhutilMissingSymbolException` from `class_exists` or `interface_exists`.Nov 24 2015, 9:34 PM

joshuaspence updated this object.

joshuaspence edited edge metadata.

Cool. This definitely feels little hacky, but I think we're ultimately working around somewhat-questionable behavior in third-party libraries in every case anyway and this may actually get us the result we desire without needing any weird dummy autoload methods anywhere or any special behaviors or an alternate include file or whatever.

src/__phutil_library_init__.php
38	This might need an: if (empty($backtrace['function'])) { continue; } ...first -- I'm not sure every stack frame is guaranteed to have the `function` key populated. I think I've seen some weird frames coming out of, e.g., callbacks from extensions like `preg_replace_callback()` and cURL, but might be misremembering. (We should avoid using `idx()` here because it might not have loaded yet.)

This revision is now accepted and ready to land.Nov 25 2015, 3:01 PM

For what it's worth, I don't think that there is anything wrong with the third-party code doing class_exists($x) instead of class_exists($x, false) because the two are not equivalent. I would expect the latter to always return false unless the class $x had previously been autoloaded.

For example:

<?php

require_once 'scripts/__init_script__.php';

class_exists('SomePhutilClass', false); // I think this should always return false

Check for empty

Don't throw from trait_exists

joshuaspence updated this object.Nov 29 2015, 8:49 PM

Closed by commit rPHUe65ffd33db02: Don't throw `PhutilMissingSymbolException` from `class_exists` or… (authored by joshuaspence, committed by joshuaspence). · Explain WhyNov 29 2015, 8:50 PM

This revision was automatically updated to reflect the committed changes.

The two aren't equivalent, but I imagine most callers may really mean to ask "Has this class already been loaded?", not "Does this class exist somewhere?".

The call in T8494 seems to be asking the former question, and could probably be written more precisely as class_exists(X, false) (but better still would be omitting the check entirely, as it is is meaningless, unless the library legitimately expects callers to substitute a different PhpParser\Autoloader by loading it first, in which case it should not do that).

There are probably some callers asking the second question, but I'd venture a guess that the first question is a lot more common. I think the original T1116 report was something similar to this one, as well.

joshuaspence mentioned this in T9954: Cannot change a fulfiled promise to rejected.Dec 22 2015, 5:53 AM

Don't throw `PhutilMissingSymbolException` from `class_exists` or `interface_exists`
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35315

src/__phutil_library_init__.php

Don't throw `PhutilMissingSymbolException` from `class_exists` or `interface_exists`ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 35315

src/__phutil_library_init__.php

Don't throw `PhutilMissingSymbolException` from `class_exists` or `interface_exists`
ClosedPublic
Actions

Revision Contents
Changeset List