This module may be duplicated by Libraries API.

* embeds an external library which does not follow d.o coding standards (#562876: Ensure no function names collisions happen).
* stores the library outside of sites/*/libraries.
* loads the simple_html_dom library for each request, regardless of whether it's used

Module authors considering a dependency on this module may wish to consider depending instead on Libraries and then using the Libraries API to load the simple_html_dom library as required.

Comments

xurizaemon’s picture

Title: Use Libraries API instead » Consider sites/*/libraries or use of Libraries API

There are several advantages to sites/*/libraries instead of using this module, whether or not you use the Libraries API or simply include the files from Libraries.

* removes the dependency on this module,
* avoids loading the simple_html_dom library on each request,
* and also seems more in keeping with 3rd party libraries in Drupal CVS.
* means you don't need to maintain a patched version to work around issues in this module, eg #922874: Notice: Undefined variable: function_name in include_once() (line 7 of ...simplehtmldom/simplehtmldom.module).

skilip’s picture

One big plus for this one. There's absolutely no reason to load the Simple HTML library on each page load, as it does now. When using the Library API you'll make it possible to load the Smple HTML library only when needed using drupal_add_library().

skilip’s picture

/**
 * Implements hook_libraries_info().
 * Add the PHP Simple HTML DOM Parser to Drupal's library register.
 */
function simplehtmldom_libraries_info() {
  $libraries['simplehtmldom'] = array(
    'name' => 'PHP Simple HTML DOM Parser',
    'vendor url' => 'http://simplehtmldom.sourceforge.net',
    'download url' => 'http://sourceforge.net/projects/simplehtmldom/files/',
    'version arguments' => array(
      'file' => 'simple_html_dom.php',
      'pattern' => '/Version\: ([0-9.]{1,})/',
      'lines' => 10,
    ),
    'files' => array(
      'php' => array('simple_html_dom.php'),
    ),
  );
  return $libraries;
}

alexkb’s picture

Skilip's solution is much better, but for anyone else out there trying to implement this, make sure the simple html dom library is extracted into sites/all/libraries/simplehtmldom. The folder name needs to be the same as the key in $libraries. Also, the latest 1.5 version of simple html dom, is v1.5 and this is printed differently in the php file, so the regex needs to be changed. Here's what worked for me:

function simplehtmldom_libraries_info() {
  $libraries['simplehtmldom'] = array(
    'name' => 'PHP Simple HTML DOM Parser',
    'vendor url' => 'http://simplehtmldom.sourceforge.net',
    'download url' => 'http://sourceforge.net/projects/simplehtmldom/files/',
    'version arguments' => array(
      'file' => 'simple_html_dom.php',
      'pattern' => '/version ([0-9\.]+)/',
      'lines' => 40,
    ),
    'files' => array(
      'php' => array('simple_html_dom.php'),
    ),
  );
  
  return $libraries;
}
Konstantin Komelin’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new136.95 KB

Provided patch includes several changes, such as:

  • Removed simplehtmdom library
  • Implemented hook_libraries_info (solution from #4)
  • Added README.txt with installation instructions
  • No dependency added, the module will work nicely w/o Library API as well as with it.

Please review.

P.S. Please use git apply --index to apply the patch.

Konstantin Komelin’s picture

If you don't have time please give me necessary permissions and I'll do everything myself.

Konstantin Komelin’s picture

Commited patch #5 04d9593 with minor text modifications.
New release 7.x-2.x created https://drupal.org/node/2187329

Konstantin Komelin’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.