Comments for "Services_OpenSearch"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • Matthew Weier O'Phinney  [2005-12-28 20:47 UTC]

    For forward compatibility with PHP >= 5, I'd suggest using a factory for instantiation; constructors in PHP >=5 cannot return an object instance of another class. So, the following will cause errors in PHP5:

    function Services_OpenSearch($url = null)
    if (! is_null($url)) {
    $this->_descriptionUrl = $url;
    } else {
    return PEAR::raiseError('OpenSearch: missing URL.');

    // ...

    Do a factory or singleton instead.

    Also, it would be nice if your constructor/factory/singleton would allow setting the various parameters of the $pager_param array. You could do this with an optional second argument:

    var $pager_defaults = array(
    'count' => 10,
    'startIndex' => 1,
    'startPage' => 1,
    'totalResults' => -1,
    'itemsPerPage' => -1

    function construct($url = null, $pager_params = array())
    // check for url
    // ...

    // Set pager params
    if (!empty($pager_params)) {
    $this->pager_param = $pager_params;

    // Get defaults for unset pager parameters
    foreach ($this->pager_defaults as $key => $default) {
    if (empty($this->pager_param[$key])) {
    $this->pager_param[$key] = $default;

    Finally, please follow CS conventions, particularly the 'one true brace' convention. The following:

    function getCount() { return $this->pager_param['count'] = $n; }

    while succinct and valid PHP, breaks from PEAR CS, which would have you format that as:

    function getCount()
    return $this->pager_param['count'] = $n;
  • bertrand Gugger  [2005-12-31 08:08 UTC]

    It's a clean integration of several pear packages.
    * package's name: should be Services_OpenSearch as I guess it should be in the Services category
    * CS for methods, even if very short methods should not be packed on one line, but rather have their block starting on next line as:
    function getAdultContent()
    return $this->_getDescription('AdultContent');
    * Shouldn't private methods names start with an underscore ?
    * It should be possible to change the url and not need to instanciate a new one for it, perhaps that could be combined to solve Matthew's question.
    * That would be nice to be able to give the pager's options directly by instanciation as an options array
    * I would check the descrition returned in fetchDescription($url) has effectively the '_got' element as it's a key for next extractions and not rely on the data correctness
    * Not sure if some cache mechanism could not save some requests here ...
    * In setupQuery(), your use of str_replace() is incorrect, both search and replace are simple arrays, the hash for replace has no meaning, only element's order is important.
  • HIROSE Masaaki  [2006-01-04 15:13 UTC]

    Thanks for great advise.

    I accept all comment and try to modify Services_OpenSearch. A little time please.