seems like there are a few things that need updating. First, we need to get the module to actually work with the current functionality. Second, we should evaluate the hooks we are providing removing any that are obsolete and adding new ones. Finally, we probably want to produce .info and .install files for folks.

CommentFileSizeAuthor
#10 module_builder_5x_patch_4.txt14.63 KBbinduwavell
#9 module_builder_5x_patch_3.txt14.12 KBbinduwavell
#7 module_builder_5x_patch_2.txt12.27 KBbinduwavell
#2 module_builder_5x_patch_1.txt8.68 KBbinduwavell
module_builder_5x_patch_0.txt7.48 KBbinduwavell

Comments

binduwavell’s picture

binduwavell commented

the original patch is absolutely not complete, but it is as far as I have gotten so far. As a newb I'm running into stuff I don't know how to fix. So I'm providing it so other can look and see if they can progress some. I will keep working on this, if I make progress I'll attach subsequent patches.

binduwavell’s picture

binduwavell commented
StatusFileSize
new module_builder_5x_patch_1.txt8.68 KB

ok, got the main page to generate, it seems to work as long as you uncheck the "Automatically generate module file for download?" checkbox. Still need to get the api-downloader to work without timing out, and to evaluate new/old hooks and get the module to generate .info and .install files... Am thinking of removing the direct download option for now as we need to generate more than one file now. In the long run we could produce a page with links that allow the user to download the files... so they can right click and do saveas rather than having copy paste...

binduwavell’s picture

binduwavell commented

The "administer >> modules" and "administer >> help >> module_name" links on the builder page are broken because of the new security features in the form api. So that will need fixing too.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Oh, wow! Thanks a lot! I'll try and look at this this weekend.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs work

I'm going to leave a detailed review here, because Bindu is part of the Drupal Dojo, and learning how to contribute. :)

General stuff:

  1. When you post a patch, even if it's not perfect, you should choose a status that indicates it's a patch (in this case, "patch (code needs work)" -- this will usually catch the eye of a busy maintainer so they pay closer attention to the issue (as opposed to ignoring it like they sometimes do with support requests :P).
  2. The patch contains CRs, probably because you've created it with a Windows program. Look for an option there to use "Unix" newlines instead of Windows.

Code-specific stuff:

  1. Because the only thing in module_builder_help() was the module description which is now covered by the .info file, you can actually remove that function altogether.
  2. if ($_POST['op'] == t('Update')) {

    You should avoid using $_POST directly if possible, and instead use $form_values['op'] (not sure if it _is_ possible here, but...). I like this addition, btw, and plan to back-port it to the 4.7 version. 

  3. +  drupal_add_js($path . '/includes/module_builder.js');

    Very minor nitpick: the concatenation period should always go next to the quote. Coding standards 

  4. +4. The first time you visit the module builder form, the module will 
+   retrieve hook documentation from cvs.drupal.org and store it locally. 
+   When you want to update this documentation later on, return to the 
+   settings page and click the "Update" button.

    This is actually no longer true with your check against $op == t('Update'). 

  5. +; $Id$
+name = Module Builder
+description = Builds scaffolding for custom modules.
+; dependencies =
+package = Development
+version = "$Name$"

    1. There are no dependencies, so you can leave that line off.
    2. "Package" should only be used when the module is part of a larger set of modules; for example CCK fields or modules that extend what Organic Groups can do. See http://drupal.org/node/101009 for more info.

Some other issues I found:
- The broken links that you mentioned.
- The description of the file field has "<em>files</em>" literally in the string; this is because it both is using the %placeholder style and has theme('placeholder') -- the theme('placeholder') should go.
- The lines to download the hook documentation are commented out, so threfore the module won't work. ;) Was this giving you trouble, or why did this get commented out?

At any rate, a great first stab at porting this and will make the other issues easy to track down. Thanks!!

binduwavell’s picture

binduwavell commented

General stuff: makes sense

Code-specific stuff:
1. I think that without module_builder_help() like that you don't get a description on one of the admin overview pages.
2. I think you (or some other contributor) did that, I changed the format to not create the op variable above when it was only used on that line. I was not able to figure out how to remove that piece of code and I did look at it for a while.
3. Funny, I read that page several times and it wasn't clear to me. Went back and re-read it and of course it makes perfect sense :)
5. I left the dependencies in there as a reminder to include that with a comment in the generated .info file. I put the package in there so that Module Builder would sort next to the devel and code review modules.

Some other issues you found:
- I'm not really up on theming yet, but I think I can figure this out if I look at it a bit
- The lines to download the hook docs are commented out because each call to drupal_http_request() takes 10-20 seconds and there is some global setting that makes the page fail to load if it takes longer than 60 seconds to load. Seems like there is an issue with drupal_http_request() taking that long to load a page which when opened in a browser on my dev box takes less than 1 second.

binduwavell’s picture

binduwavell commented
StatusFileSize
new module_builder_5x_patch_2.txt12.27 KB

General stuff:
2. Please let me know if I have resolved this with the latest attachment.

Code-specific stuff:
1. Either this was fixed in a recent commit, or I was wrong, I have removed hook_help()
2. I have been unable to find another way to get at 'op'
3. Fixed
4. I think the call to _module_builder_check_settings() at the top of module_builder_page() would result in the hook documentation getting retrieved the first time you goto this page.
5. I removed dependencies and package. I know I don't get a vote, but :) I think that there should be a Developer package (I saw that the devel module just removed this... there are a few modules for developers, and I personally like having them grouped together..) What do you think?

Some other issues I found:
- Fixed the links (used ! instead of % for placeholders)
- removed explicit emphasis from path display
- looks like the drupal_http_request method is now working great, yay :)

binduwavell’s picture

binduwavell commented

minor correction in 5. above I say I saw that the devel module removed the Development package, actually I don't think they have, the coder package appears to have removed it.

binduwavell’s picture

binduwavell commented
StatusFileSize
new module_builder_5x_patch_3.txt14.12 KB

Fixed some string formatting issues from the old code discovered by the coder module. The only remaining item from coder is our uses of $_POST['op'] in the settings method. I had an idea about how to fix it, but don't have time tonight to give it a shot.

binduwavell’s picture

binduwavell commented
Status: Needs work » Needs review
StatusFileSize
new module_builder_5x_patch_4.txt14.63 KB

Fixed the only remaining coder report. Still need to consider adding drupal 5 features (like .info file generation) but I think this is code complete on the conversion front.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Awesome. Committed to HEAD; thanks!

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)