Re: [PEPr] Images::Imagick proposal, Small Errors

> proposal: > http://pear.php.net/pepr/pepr-proposal-show.php?id=64 > > I also hope to get some input on "Image_Magick_Conjure" by Florent Monnier. > He doesn't use a PEAR-Imagick-Api right now and it would be nice to get > together somehow. =========== Here are one or two small errors that should be corrected: - there are some small coqs: @ccess @access IMAGE_IMAGICK_ERROR_RORATE_FAILED IMAGE_IMAGICK_ERROR_ROTATE_FAILED Moreover this constant is not used anywhere, like the other ones. - You forgave HTML debug code in addMotionBlur() in the alpha 0.2 - in loadImage() function: the second parameter for substr() should be the string length, rather than the end position: $_sImageName = substr($_sImage, $mTmpPos_1 + 1, $mTmpPos_2 - $mTmpPos_1 - 1); An there's also a trick on this, sometimes on the web, people don't put any file extention at all. The more "avant-garde" for this are W3C ;-) http://www.w3.org/Icons/w3c_home http://www.w3.org/Icons/WWW/html_48x48 http://www.w3.org/Icons/valid-xhtml11 And if the filename does have a dot in its name? Yes I know there are a lot of 'if', but the case is possible. (The php func pathinfo() cannot handle this case neither.) far more greedy but should work: preg_match('!([^/]+?)\.\w{1,5}$!', $this->_sImage, $m); // or an ereg version $this->_sImageName = $m[1]; If you shoose to not handle tricky cases, perhaps put a warning in the doc? to prevent users don't put dots in filenames if they don't put file ext. (This one won't be 100% perfect too, the only ultim way would put all extentions in an array and comparing with it.) In the function addUserFilter(), your include won't work in case that somefile.php does not exists if you strip the '@': (as you propose it to be in pear coding standard) I think you should put it back this way, for this code to work as you expect: $bCheckReq = @include_once 'somefile.php'; if (!$bCheckReq) { // error handling } If you really want to remove all '@' from your code, perhaps another solution would be to use: if(file_exists() && is_readable()) {/*safe include*/} -- Best Regards Florent