Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#35 | discogs.import.tar_.gz | 1.14 KB | Karlheinz |
#30 | discogs.tar_.gz | 21.65 KB | Karlheinz |
#20 | discogs.zip | 20.93 KB | Karlheinz |
#14 | discogs0.02.zip | 21.04 KB | Karlheinz |
#1 | discogs.zip | 21.13 KB | Karlheinz |
Comments
Comment #1
Karlheinz CreditAttribution: Karlheinz commentedAttached zip-compressed file of module code.
Comment #2
Karlheinz CreditAttribution: Karlheinz commentedForgot to change the status, sorry.
Comment #3
apadernoHello, 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.
Comment #4
Karlheinz CreditAttribution: Karlheinz commentedSorry, I guess I can't edit my original ticket. Here is a more detailed motivational message:
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?
Comment #5
apadernoThat link is for users with the right permission; it's perfectly normal you are not able to access it.
Comment #6
Karlheinz CreditAttribution: Karlheinz commentedFair 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.
Comment #7
Karlheinz CreditAttribution: Karlheinz commentedI 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."
Comment #8
Karlheinz CreditAttribution: Karlheinz commentedAny way I can help this process along?
Comment #9
apadernoThe 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 tot($variable)
; this means that if the argument of the function is not a literal string, it will not appear in the translation template.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.
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.The function seems to do something similar to what
truncate_utf8()
does, with the difference that it's not Unicode safe.Drupal will think the function is the implemention of
hook_search()
, with the effect to produce runtime errors.There is a Drupal function that should be used in these cases.
Runtimes errors should be reported in
hook_requirements()
.Comment #10
Karlheinz CreditAttribution: Karlheinz commentedThanks 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
orhook_perm()
. I assumed it was necessary to add them by hand. I left a comment about this on thehook_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 calledtruncate_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.Comment #11
apadernoComment #12
Karlheinz CreditAttribution: Karlheinz commentedHey, why was this closed?
I'm waiting for someone to answer my questions before I upload more code.
Comment #13
apadernoI apologize; the status was not set to the correct one.
Comment #14
Karlheinz CreditAttribution: Karlheinz commentedOkay, 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" usingvariable_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.)Comment #15
apadernoVerify how the control structures are formatted; the curly brackets are not optionals, for the Drupal coding standards.
drupal_http_request()
.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.Comment #16
Karlheinz CreditAttribution: Karlheinz commentedOkay, 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.
Comment #17
apadernoWhat reported from the coding standards is also valid for the PHP
(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()
usesfwrite()
, andfread()
; AFAIK,fread()
is able to read compressed data; the function allows you to add extra headers, if you need them.Comment #18
apadernoI am changing the status as per point #3 reported in comment #16.
Comment #19
Karlheinz CreditAttribution: Karlheinz commentedI 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 usehook_requirements()
at all.Comment #20
Karlheinz CreditAttribution: Karlheinz commentedI 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 bymodule_load_include()
.4.
_discogs_trim()
is now mostly a wrapper fortruncate_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 todiscogs_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.
Comment #21
Karlheinz CreditAttribution: Karlheinz commentedChanged status.
Comment #22
apadernoComment #23
sreynen CreditAttribution: sreynen commentedThis 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:
Comment #24
Karlheinz CreditAttribution: Karlheinz commentedHey, 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?
Comment #25
apadernohttp://drupal.org/project/issues/subscribe-mail/cvsapplications: Select Own issues.
Comment #26
strawberrybrick CreditAttribution: strawberrybrick commentedcomplete 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
Comment #27
Karlheinz CreditAttribution: Karlheinz commentedone 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.Comment #28
apadernoI 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.
Comment #29
strawberrybrick CreditAttribution: strawberrybrick commentedThat would be helpful, thanks
Comment #30
Karlheinz CreditAttribution: Karlheinz commentedOkay, a new version is ready.
However, there are a couple things I did NOT get.
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.
Comment #31
sreynen CreditAttribution: sreynen commentedThis 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.
Comment #32
apadernoThank 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.
Comment #33
Karlheinz CreditAttribution: Karlheinz commentedAwesome, thank you very much.
Can't wait to get this out to the folks.
Comment #34
Robert Gomez CreditAttribution: Robert Gomez commentedI 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.
Comment #35
Karlheinz CreditAttribution: Karlheinz commentedFatal 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.
Comment #36
Robert Gomez CreditAttribution: Robert Gomez commentedPerfect. It's working now.
Comment #37
Karlheinz CreditAttribution: Karlheinz commentedPerfect. 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?
Comment #38
sreynen CreditAttribution: sreynen commented@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.
Comment #39
Karlheinz CreditAttribution: Karlheinz commented@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.
Comment #42
apaderno