The patch that went in at #273137: Split Navigation to User and Administration menu added some new menu blocks to system.module, but the settings for the "Powered by Drupal" block are now showing up on the configuration screen of these blocks (see attached screenshot).

CommentFileSizeAuthor
#10 core_system_d7-menu-blocks-configuration-help-410638-10.patch845 bytesfoxtrotcharlie
#2 core_system_d7-menu-blocks-configuration-410638-2.patch3.26 KBfoxtrotcharlie
Screenshot.png96.38 KBDavid_Rothstein

Comments

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +Novice

While not an "extreme novice" patch, it seems like this would be a good patch for someone to work on if they are interested in learning more about the various hook_block_op() functions in Drupal 7.

(Basically, it looks like the bug arose because some of the existing hook_block functions in system.module were not updated to reflect the new blocks that were added. So fixing this bug is a good way to learn about how new blocks are/should be added to the codebase.)

foxtrotcharlie’s picture

foxtrotcharlie commented
Status: Active » Needs review
StatusFileSize
new core_system_d7-menu-blocks-configuration-410638-2.patch3.26 KB

My second ever patch, first core patch!

This patch fixes the issue where the "powered-by" Drupal block's configuration settings were showing up in the configure screen for the system menu blocks. The powered by Drupal configuration settings now only show if the block $delta == 'powered-by'.

Is this a good enough description of the patch?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

Looks fine to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Hey, great catch, foxtrotcharlie! :) Yep, your patch name looks great and conforms to coding standards, rolled from Drupal root, etc. Way to go!

Committed to HEAD. Oopsie. :D

foxtrotcharlie’s picture

foxtrotcharlie commented

Thanks webchick :-) I was listening to the latest lullabot podcast where you mentioned the novice patches, and well, the rest is history ;-)

David_Rothstein’s picture

David_Rothstein commented
Status: Fixed » Needs work

Nice work! One thing missing, though - you can see from the above screenshot that there is help text showing at the top of the page (that describes what the "powered by Drupal" block is). That wasn't fixed by the patch that was committed, so it's still showing up on the configuration page for all the new menu blocks.

Would you be interested in trying to fix that also?

foxtrotcharlie’s picture

foxtrotcharlie commented

Damn - was too eager! Will roll a new patch shortly.

foxtrotcharlie’s picture

foxtrotcharlie commented

One more thing - I assume that the new patch is rolled against my already patched version, as it's already been checked into CVS ?

webchick’s picture

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

Correct. You'd do:

cvs up -dPC

to grab the latest code from CVS

And then make your modifications and roll a patch against that.

foxtrotcharlie’s picture

foxtrotcharlie commented
Status: Needs work » Needs review
StatusFileSize
new core_system_d7-menu-blocks-configuration-help-410638-10.patch845 bytes

Removed the "Powered by Drupal" help text that was appearing in the system defined menu blocks. This help text should now only appear on the configuration page for the "Powered by Drupal" block.

David_Rothstein’s picture

David_Rothstein commented
Status: Needs review » Reviewed & tested by the community

Yup, that should do it! PHP was just happily evaluating any string to be equal to the integer 0 :) So this is actually an older bug that wasn't revealed until now (since up until now, the powered by Drupal block was the only one provided by system.module).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Excellent! Committed this, too. Thanks, folks! :)

foxtrotcharlie’s picture

foxtrotcharlie commented

Thanks David_Rothstein, webchick and catch for your help. Looking forward to contributing more :-)

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Novice

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

Issue tags: +Quick fix, +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.