CVS edit link for Karlheinz

I have created a Discogs module. It creates a Discography content type, and can import information from Discogs.com via their REST interface. It also has image support: the Image module's image_attach, CCK imagefields, and core uploads are supported, and it has its own (very basic) image handler if none of the former are available. All new fields are integerated with Views 2.

I have been searching for a similar Drupal module, but didn't find one, so I believe this is original functionality. I know that others are looking for a module like this, and I want to make this available to the community.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Karlheinz’s picture

FileSize
21.13 KB

Attached zip-compressed file of module code.

Karlheinz’s picture

Status: Postponed (maintainer needs more info) » Needs review

Forgot to change the status, sorry.

apaderno’s picture

Status: Needs review » Needs work
Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

Karlheinz’s picture

Sorry, I guess I can't edit my original ticket. Here is a more detailed motivational message:

I have created a new module called "discogs."

The module creates a new content type, called "Discography." That content type has fields for artist, label, catalog number, format, country, date, and album credits. In addition, it can contain multiple tracks (which reside in their own database table in a many-to-one relationship). Tracks have fields for position, title, artist, duration, notes, and lyrics.

The "atomic" values for each node (i.e. not tracks) are all available through the Views interface.

In addition to creating this content type, this module also can pull in discography data from www.discogs.com. A developer API key is needed (it defaults to my own). Through a sub-menu of the discography menu, you can search for artist, album, or label data, returning a checklist of albums, which can then be imported into Drupal as discography nodes.

Images can be handled in four ways:

  1. CCK Imagefield
  2. Image module's image_attach
  3. Core upload module
  4. If none of the above are available, a "native" image handler

Upon install, the discogs module will check in that order to see what is available, and automatically activate that option if it is. This can be changed later through the Discogs administration panel.

The "native" image handler is just an internal path to an image file, and a path to its thumbnail. By default these are put into the files directory, but that may be changed. These paths are stored in the database with the node's field information. The "native" image handler is there mostly to handle Discogs.com imports; it does not have e.g. an upload function in the user interface.

Comparison with existing modules:

You could create this content type using CCK and custom fields. (From experience, I can tell you that this is not as easy as you would think, particulary with regards to the tracks.)

But as far as I know, there are no available modules, nor combination of modules, that would allow you to import data from Discogs.com. This was actually the reason I created this module.

Hope that does it.

Incidentally - I saw the edit link in the ticket. If I attempt to follow that link, it says I am not authorized. Should this be the case?

apaderno’s picture

Incidentally - I saw the edit link in the ticket. If I attempt to follow that link, it says I am not authorized. Should this be the case?

That link is for users with the right permission; it's perfectly normal you are not able to access it.

Karlheinz’s picture

That link is for users with the right permission; it's perfectly normal you are not able to access it.

Fair enough. I was just wondering if I was supposed to have access (as the module author), or if it was for e.g. the reviewers only.

Sounds like the latter. I was just a bit confused about the process, that's all - it's my first module on Drupal.org.

Let me know if you need anything more from me.

Karlheinz’s picture

Status: Needs work » Needs review

I didn't realize that kiamlaluno changed the status to "needs work." I assume he meant the motivational message. Since I changed that, I'm changing the status back to "needs review."

Karlheinz’s picture

Any way I can help this process along?

apaderno’s picture

Assigned: Unassigned » apaderno
Status: Needs review » Needs work
  • The points reported in this review are not in order or importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1.       return t('The discography content type holds information about a '
            . 'discography release. This may either be entered by hand, or '
            . 'imported from the database at Discogs.com. In order to access the '
            . 'Discogs database, you must enter a Discogs API key in the '
            . '<a href="@url">Discogs settings</a> page. You should also enter '
            . 'your preferred module for image handling. Currently supported '
            . 'modules are: <a href="@ifurl">CCK Imagefield</a>; the '
            . 'image_attach component from the <a href="@iurl">Image module</a>; '
            . 'the core upload module; and a "native" image handling system '
            . "that merely stores local URL's to images. Upon installation, the "
            . 'Discography module automatically detected the presence of these '
            . 'modules, and set a default according to the order above. '
            . 'WARNING: Although you can switch image handlers, images themselves '
            . 'will NOT be transferred from one image handling system to another. '
            . 'Also, if you are using the Image module, you might want to go to '
            . 'the <a href="@icurl">Images configuration page</a>, and change the '
            . "thumbnail size to 150x150px to match Discogs.com's thumbnail size.",
            array(
              '@url'   => url('admin/settings/discogs'),
              '@icurl' => url('admin/settings/image'),
              '@ifurl' => 'http://drupal.org/project/imagefield',
              '@iurl'  => 'http://drupal.org/project/image',
            ));
    

    The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

  2. function discogs_perm() {
      return array(
        'create discography entry',
        'delete own discography entry',
        'delete any discography entry',
        'edit own discography entry',
        'edit any discography entry',
        'import from Discogs'
      );
    }
    

    For any content type, the module node.module already defines the first permissions, that should not be declared also from the module, or administrator users would probably see the same permissions twice.

  3.   if (module_exists('imagefield')) {
        include_once('discogs.admin.inc');
        discogs_cck_imagefield();
      }
    

    As the directory set as current directory is the Drupal root directory, include is trying to load a file that will not find. There is a Drupal function that is thought for these cases.

  4. function _discogs_trim($string, $length = 0, $strip = TRUE) {
      $string = trim($string);
      if (($length > 0) && (strlen($string) > $length)) {
        $last       = substr($string, 0 , $length);
        $last_space = strrpos($last, ' ') ? strrpos($last, ' ') : $length;
        $string     = substr($string, 0 , $last_space);
      }
      $string = $strip ? strip_tags($string) : $string;
      return $string;
    }
    

    The function seems to do something similar to what truncate_utf8() does, with the difference that it's not Unicode safe.

  5. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how functions, global variables, and Drupal variables defined from the module should be named; how the code should be formatted.
  6. function discogs_search($term, $type = 'release', $api_key = '') {
      if (empty($api_key)) {
        $error = t('You need to enter a Discogs API key into the '
          . '<a href="@url">Discogs settings</a>.',
          array('@url' => url('admin/settings/discogs')));
        drupal_set_message($error, 'error');
        return FALSE;
      }
      $xml_array = array();
      $term = (strpos($term, ' ') === FALSE) ? $term : urlencode($term);
      $url = 'http://www.discogs.com/search?type=all'
        . "&q=$term&f=xml&api_key=$api_key";
      $xml_string = _discogs_fetch_url($url);
      if ($xml_string) {
        $ret_array = _xml_to_array($xml_string, "resp");
        $num_results = $ret_array[0]['searchresults']['numResults'];
        if ($num_results > 0) {
          // Combine exact matches with search results
          $exact = is_array($ret_array[0]['exactresults']['result'])
            ? $ret_array[0]['exactresults']['result']
            : array();
          $search = is_array($ret_array[0]['searchresults']['result'])
            ? $ret_array[0]['searchresults']['result']
            : array();
          $results = array_merge($exact, $search);
    
          // Get ID's of results that match the type
          foreach ($results as $result){
            if ($result['type'] == $type) {
              $uri = parse_url($result['uri']);
              $id = basename($uri['path']);
              $xml_array[$id] = $result;
            }
          }
        }
      }
      else {
        $error = t('Error parsing Discogs XML data from %url.',
          array('%url' => $url));
        drupal_set_message($error, 'error');
        return FALSE;
      }
      return $xml_array;
    }
    

    Drupal will think the function is the implemention of hook_search(), with the effect to produce runtime errors.

  7. function _discogs_fetch_url($url) {
      // Create a stream
      $context = stream_context_create(array(
        'http' => array(
          'method' => 'GET',
          'header' => 'Accept-Encoding: gzip',
        )
      ));
      // The returned XML is sometimes gzipped, sometimes not
      if ($zp = @gzopen($url, 'r')) {
        $contents = gzread($zp, 1048576);
        gzclose($zp);
        return $contents;
      }
      elseif ($contents = @file_get_contents($url, FALSE, $context)) {
        return $contents;
      }
      else {
        return FALSE;
      }
    }
    

    There is a Drupal function that should be used in these cases.

  8.   else {
        // Neither DOM nor DOM XML is available
        $error = t('Either DOM or DOM XML must be installed to query Discogs.');
        drupal_set_message($error, 'error');
        return FALSE;
      }
    

    Runtimes errors should be reported in hook_requirements().

Karlheinz’s picture

Thanks for the review. I will get to work on changing the code. I do have some questions:

1. Is there any way to write the t() function so that it won't be an insanely long line? I'd rather not have to scroll for days just to read that line of code. (Eclipse, my IDE, doesn't wrap lines.)

2. There's no mention of this in the documentation for page_example.module or hook_perm(). I assumed it was necessary to add them by hand. I left a comment about this on the hook_perm() API page.

3. I made some slight changes after I uploaded the .zip file, this might be one of them. If not, I'll update it to use module_load_include().

4. I didn't realize that truncate_utf8() could truncate on word boundaries. However, truncate_utf8() doesn't strip HTML tags, and the usage is somewhate different. Would it be acceptable if I just called truncate_utf8() from within _discogs_trim()?

5. Hm, I thought I was using coding standards? Am I missing something? Should I not be breaking lines at 72 characters for readability?

6. Good call, I'll change that.

7. What is that Drupal function? I couldn't find one, but I'm not even sure where to look. I had to write this specifically to handle the wonky way Discogs returns its XML, so I don't know if the Drupal function would work...?

8. Can hook_requirements() handle either/or situations? One or the other needs to be installed, not both.

apaderno’s picture

Status: Needs work » Closed (won't fix)
Karlheinz’s picture

Status: Closed (won't fix) » Postponed (maintainer needs more info)

Hey, why was this closed?

I'm waiting for someone to answer my questions before I upload more code.

apaderno’s picture

Status: Postponed (maintainer needs more info) » Needs review

I apologize; the status was not set to the correct one.

Karlheinz’s picture

Status: Needs review » Active
FileSize
21.04 KB

Okay, obviously I should have made some changes and uploaded the code, even if I couldn't make all the changes without more information.

Attached an updated .zip file with slightly new code, and changed the status to "active."

It would help immensely if you could give me some specific answers to #5, #7 and #8.

For #8, I guess I would create hook_requirements() inside the .install file, which checks to see which DOM functions are available, and sets a "flag" using variable_set()? That's the only thing I could think of, but I'm not sure I'm correct. (I have not made this change in the code yet.)

apaderno’s picture

Status: Active » Needs review
  1. Verify that any Drupal variable, global PHP variable, and PHP function names are prefixed with the module name.
    Verify how the control structures are formatted; the curly brackets are not optionals, for the Drupal coding standards.
  2. The function I was referring to is drupal_http_request().
  3. The module should implement hook_requirements() and verify if the functions the module requires are present; no warning should be given for the functions that the module uses when they are present, but they are not strictly necessary for the module to work.
Karlheinz’s picture

Okay, sorry for the extra questioning.

1. Does this also apply to (private) helper functions? PHP convention indicates that they start with an underscore - should I do an underscore followed by the module name?

2. The XML data from Discogs.com might be gzipped, or it might not, even though the request always requires the "Accept-Encoding: gzip" header. However, drupal_http_request() looks like it can't parse gzipped data. This is why I wrote my own function. Am I wrong about this?

3. So, I guess that's a "yes," then. I'll get on it.

apaderno’s picture

What reported from the coding standards is also valid for the PHP private functions (which are indeed prefixed by the underscore); the reason to include the module name in the function name is to avoid conflicts between projects.

drupal_http_request() uses fwrite(), and fread(); AFAIK, fread() is able to read compressed data; the function allows you to add extra headers, if you need them.

apaderno’s picture

Status: Needs review » Needs work

I am changing the status as per point #3 reported in comment #16.

Karlheinz’s picture

I am working on it right now.

Just so you know, drupal_http_request() does NOT handle gzipped data, apparently.

EDIT: I was totally wrong when I said that. It was a problem with Discogs not returning data for some reason.

Also, I have found a way to parse the XML data into a multidimensional array using only xml_parser functions, which are core PHP functions. That means it's not dependent upon either DOM or DOM XML, so I won't even need to use hook_requirements() at all.

Karlheinz’s picture

FileSize
20.93 KB

I believe I have finished all of the required changes:

1. t() calls only contain single string literals.

2. hook_perm() does not include default permissions (which are added automatically).

3. include() calls replaced by module_load_include().

4._discogs_trim() is now mostly a wrapper for truncate_utf8().

5. Function names are now all prefixed by "discogs" or "_discogs." All functions are commented. The only thing I did not fix is conditional assingments (using the "statement ? T : F" format). I still have some that are not on a single line. I don't know if this actually breaks coding standards or not, but it makes them readable in IDE's that do not wrap lines.

6. discogs_search() renamed to discogs_search_for().

7. All REST calls to Discogs now use drupal_http_request().

8. I wrote a new XML parser that uses the xml_parser functions (native to PHP4 and PHP5), thus there is no dependence on DOM or DOM XML. So hook_requirements() is no longer required.

I also made some minor improvements to the code, such as more information on the final stage of the import form.

Please let me know if you find anything else I should fix, or if you have any suggestions. I'd like to get this out to the community sooner rather than later.

Karlheinz’s picture

Status: Needs work » Needs review

Changed status.

apaderno’s picture

Assigned: apaderno » Unassigned
sreynen’s picture

Status: Needs review » Reviewed & tested by the community

This looks like a really useful module. I just stumbled on it in a search for documentation on accepting gzip with drupal_http_request, and I'm happy I did as I can see where I might use this. It pulls in a ton of great data from discogs.com.

I think it's ready to go into CVS. I found some minor issues, but nothing that can't be fixed after it's in CVS:

  • On install, there's a message to change settings, but no indication of how to use the module after that. A project page would help with that, but a README.txt would also be nice. I clicked on the Discography link in the admin menu (that should probably go under Content) and got the "No discography releases have been added yet." message. That seems like a natural place to link to the import page, which I eventually found.
  • Running Coder on it comes up with 45 warnings, which all seem to be minor coding standards issues, e.g. missing spaces.
  • There are a few PHP notices, which don't show up on a normal Drupal install, but do show up if you apply the core patch from SimpleTest.
  • The API key field has a value on install. If that's intended to stay there, maybe add some explanation. It looks like I'm using someone else's API key, which feels wrong.
Karlheinz’s picture

Status: Reviewed & tested by the community » Needs work

Hey, thanks for the comments. I'll integrate your suggestions ASAP.

- A readme.txt is a good idea. Even better would be to also integrate it with Advanced Help. I'll get on that.

- The "Discography" menu link is actually just a link to the discography section of the site. It's there for people who don't have Views installed. (I'm trying to make this module have no non-core dependencies.) You're right, though, "add" and "import" links would be really useful.
EDIT: I have it running on a site where that is overridden by Views, and even though plenty of releases have been imported, it still gives me that message. That is definitely an error. I'll look into that. (Since everyone and their mother uses Views already, maybe I should just get rid of it.)

- I haven't yet set up the Coder or SimpleTest modules (I'm still hazy on unit testing with PHP). I've been meaning to do that anyway, so it seems like now is the right time. (The missing spaces is odd; I use Eclipse, which strips all trailing spaces on save.)

- The API key is my own. You need an API key to get the data; and requiring the user to get one would mean the module wouldn't be able to import anything until they get one. The only issue is that "API usage is limited to 5,000 requests per 24-hour period, per IP address." I can't see it going over that limit, even on a thousand Drupal installs. Nonetheless, I will make a recommendation to apply for one in the documentation (it's already recommended on the page where you input the key itself).

EDIT: I would also like to make a couple more slight changes. For example, the "Discogs import" link would work better under the "Create Content" menu, rather than the "Administer->Content Management" menu.

I'll make these changes. Until I do, I'm going to go ahead and set the status back to "needs work."

p.s. Is there a way for me to get emailed when this issue gets updated?

apaderno’s picture

strawberrybrick’s picture

complete newb here, but I just installed drupal and the discogs api and it's really amazing. one thing I was looking for in particular, was being able to request a *specific* release by it's ID number, e.g. 485319.
If I do an import with a number like that, it returns nothing.

http://www.discogs.com/search?type=release&q=r:485319?f=xml&api_key=

thanks for any help
c

Karlheinz’s picture

one thing I was looking for in particular, was being able to request a *specific* release by it's ID number, e.g. 485319.

I actually had something like this working on a very early iteration of the Discogs module. If you put the ID number in and set the search type to "Release," it would get the specific release from Discogs. I'll see about putting that back in (though I should probably make it a separate search type).

Incidentally - I found out that the advice given to me by kiamlaluno in #9, point #3, appears to be wrong. The node.module does NOT automatically add the standard permissions. I'm going to add those back in as well.

apaderno’s picture

Incidentally - I found out that the advice given to me by kiamlaluno in #9, point #3, appears to be wrong.

I apologize for my mistake (comment #9, point #2). node.module creates the permissions only when it is associated with the content type; that is what happens with book.module, which in fact doesn't define the Edit own books permission.

strawberrybrick’s picture

I'll see about putting that back in (though I should probably make it a separate search type).

That would be helpful, thanks

Karlheinz’s picture

Status: Needs work » Needs review
FileSize
21.65 KB

Okay, a new version is ready.

  • Created a README.txt file.
  • "Import from Discogs.com" is now in the "Create Content" menu where it belongs.
  • Entering the specific Release ID will now import that release. (This was never taken out actually, it was a bug.)
  • Fixed the default permissions mix-up. (No hard feelings, Kiamlaluno.)
  • When you haven't created any Discography nodes, it now presents you with links to create some.

However, there are a couple things I did NOT get.

  • The default discography menu disappearing. I could not re-create this bug. It must have been something in an older version, I guess.
  • If you try to enter an album by hand (not through importing from Discogs.com), the "Release Title" field floats BELOW all the custom fields. Go to node/add/discogs and you'll see what I mean.

    This one is a total mystery, because it was not happening before. As expected, the "Release Title" is simply the node title. I've already set the weight down to -99 in my implementation of hook_form(). Does anyone know what's going on?

Once the above issue is worked out, I think it should be ready to go. Tell me what you think, or if you have any advice.

sreynen’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. I'm not seeing the "Release Title" below the other fields. It must be something specific to your local Drupal doing that.

Even if that is a problem in certain circumstances, it's only an interface issue, not a functional issue, so it's not a reason to hold up this CVS application. This thread is starting to look like an issue queue (there's even a feature request), so I think it's time to give the project an actual issue queue.

apaderno’s picture

Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Karlheinz’s picture

Awesome, thank you very much.

Can't wait to get this out to the folks.

Robert Gomez’s picture

I tried installing the module on a prototype site that I am working on (with djfake who commented above) and get the following error:

Fatal error: Call to undefined function discogs_cck_imagefield() in /sites/all/modules/discogs/discogs.install on line 23

I have been able to activate it on a clean install of Drupal 6.20, but my prototype site already has CCK, Views, Imagefield, Date and a handful of other modules installed. I get the above error when I attempt to activate the module on this site.

Karlheinz’s picture

FileSize
1.14 KB

Fatal error: Call to undefined function discogs_cck_imagefield() in /sites/all/modules/discogs/discogs.install on line 23

That was a stupid mistake. The function should actually be named _discogs_cck_imagefield() (note the underscore at the beginning).

I haven't finished setting up CVS, so I can't make a patch. I've uploaded the new version of discogs.install instead (compressed into tar.gz format). It's a simple fix, so it might even be quicker to make it yourself.

Robert Gomez’s picture

Perfect. It's working now.

Karlheinz’s picture

Perfect. It's working now.

Cool. Let me know if you find any other bugs.

Incidentally - to the Drupal crew: I'm having a lot of trouble logging in to my CVS account. I set my password, but the server rejects it. I've tried from a Windows machine (using both Eclipse and TortoiseSVN), and my Mac OS X laptop. Neither worked.

Does it take time to propagate, or something?

sreynen’s picture

@Karlheinz, looks like it does take time. See #211452: CVS password reset should mention a 20-30 minutes delay. If you're still having a problem, maybe open a new issue under the "User account" component in the webmasters queue.

Karlheinz’s picture

@Karlheinz, looks like it does take time. See #211452: CVS password reset should mention a 20-30 minutes delay.

It works now. Part of the problem was that the CVS username was in a different case than I thought ("karlheinz" vs. "Karlheinz"). I know usernames are case-sensitive, and tried it both ways... but only within the first 20-30 minutes after I changed the password. DOH!

The code is now in the repo as we speak.

If you're still having a problem, maybe open a new issue under the "User account" component in the webmasters queue.

Ha, you know, I didn't even know that queue existed. Thanks for that.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

apaderno’s picture

Component: Miscellaneous » new project application
Assigned: Unassigned » apaderno
Issue summary: View changes