Re: REMINDER - stream wrappers in include_path

Dmitry Stogov wrote: > > > Gregory Beaver wrote: >> Dmitry Stogov wrote: >>> REALPATH_FAILED looks like a hack :( >>> PHP itself doesn't need it at all, according to your patch >>> php_stream_open() may just return NULL immediately instead of setting >>> of this flag. >>> However I am not sure that it will work for all cases. The fact that >>> we cannot resolve the path doesn't mean that we cannot open the file. >>> (On some systems getcwd() may fail on some reasons). >> I remember Solaris having issues with this, now that you mention it. >> >> However, the place that I added this already returned NULL if realpath >> fails, it doesn't change the behavior, just moves the realpath to an >> earlier position and eliminates an unnecessary double call. However, if >> we are comfortable having the double realpath on files not found in >> include_path (this would only have a performance affect on apps with >> lots of conditional loading of drivers where the majority are not >> found), this is fine with me. I've attached a patch that removes >> REALPATH_FAILED so there are options. >> >> Another option would be to determine which OSes this can fail, and >> simply not use REALPATH_FAILED on those OSes. >>> I didn't test the patch with pecl/phar. >>> Could you explain why php goes into the endless loop? >> because the cut/paste to plain_wrapper.c:1419 needs a ptr = end, unlike >> the code in fopen_wrappers.c. > > You are right. > >> Once you fix this problem, then the real >> logic problem rears its head, which is that we go into plain_wrapper to >> open an external stream wrapper. It is far less efficient (about 4-6% >> by my measurements) to do this, which is why I moved the include_path >> parsing into streams.c > > Did you measure pecl/phar or regular files? I only measured regular files, my concern is that we don't affect any existing users, 100% of whom use regular files at the moment (obviously :). > >> This bug only happens when the last item in include_path is a stream >> wrapper and the file is not found in include_path. > > In general I don't have objections except of changing > zend_resolve_path() (which is a callback for ZE) into > php_resolve_path(). Anyway I'll look into patch once again with fresh > head. > > I would also ask Stas and other interested people to look into it. Thanks Dmitry, Greg