Permission mixup, missing max depth, & codestandards

Completely forgot to ask for review!

Hmm... looks like i've forgot to update a is_book_access to book_made_simple_is_access

Providing patch again soon.

@miro_dietiker: Thanks for the code review - i quickly looked over the patches & looks great. I'm changing the title slightly to reflect the three parts of this issue: "Permission mixup, missing max depth, & codestandards"

Let's make these three into separate issues:
1. Permission mixup
2. Missing max depth
3. Codestandards

Ideally we'd make separate patches for each issue. But I see how this could get annoying quick. Since the codestandards part of this patch basically swaps nearly every line (from tabs to spaces), the other two patches would either have to be applied to the current 6.x-2.x-dev before code cleanup, or they would need to be re-rolled to apply cleanly against dev after cleanup.

I suggest we do this in 2 steps:
* First roll a new patch against the current dev with just the codestandards part of your changes (omitting the 'Permission mixup' and 'missing max depth' changes) and I will update dev with that patch.
* Then add the following as separate patches:

--
1. Permission mixup
in book_made_simple_link():

--
2. Missing max depth
in the same function as above:

Would you be willing to do that or should I ?

Sorry for mixing.. and thanks for your positive response.
I was just checking the whole module and compared implementations to D6 books standards. That was where i've found the depth diff.
I've rerolled the patch as provided below.

BTW i've tried to merge without success...:

#cvs update -r DRUPAL-6--2-1
#patch -p0 < book_made_simple.install.patch
#patch -p0 < book_made_simple.module.patch
#cvs update -R HEAD
// leading to 100% conflicts in install and module

Then i tried to get the diff from CVS to apply it on my 2-1 version...
#cvs diff -r DRUPAL-6--2-1 -r HEAD
And seen the whole file gets outputted so applying your diffs won't work to my 2-1 version.

If you find time and know what exactly changed, i'd be happy you do the merging.
(don't forget the additional #2 replacement)

Thanks

@miro_dietiker: following the plan in #3 above, I started with the codestandards patch (against HEAD - same as 6.x-2.x-dev)

We could go farther with this, like adding @file & other standard comments, etc. But this @ least sorts out the tabbing and single-line if statements, inconsistent line breaks, etc. Like you said, the code cleanup basically re-arranges (replaces) most of the module.

I'd like to commit a version of this patch first, so all other patches can be more reasonable moving forward (including the Permission mixup & Missing max depth issues, also per #3). BTW, is #2 not addressed in #3?

Ok, so if you have time - take a look @ this patch - and let me know if you see anything else major I missed before I commit this (hopefully tomorrow).

This looks great to me as a first step.
I've compared a lot ot lines and i didn't find any mistake, but it's hard to say for sure with that kind of patch.

I've seen also some remaining non-standard lines. I'm pretty sure coder.module would have told you some more things to do. E.g. there should always be a space after comma.

http://drupal.org/coding-standards
http://drupal.org/project/coder

I also tried to address function naming conventions: You're not using module prefixes in function names. It is important to start functions with bookmadesimple_ and continuing in small letters and underscores instead of mixed case function names. Also for this - coder will tell perfectly notify you.

However, it is also great to start with that patch as-is and later on fix further tiny formattings and renamings in next steps/patches. The Code base would be already much cleaner and patches become more stable.

However, it is also great to start with that patch as-is and later on fix further tiny formattings and renamings in next steps/patches. The Code base would be already much cleaner and patches become more stable.

Yep - that's exactly what I'm thinking. If we can fix commit only the formatting fixes first - changing anything that could lead to consequences we're not thinking about now - then it's a clearer base to work from :)

I have to run this morning, but will be back later & hopefully get this going today. Thanks for the reminder and review

ok, the patch is committed to 6--2, so we'll see a new dev snapshot reflecting that in the next 12 hrs. After that, it should be much easier to patch against the remaining 2 parts of this issue.

Check administer permissions. That one was missing.
Everything else (DEPTH, perm, ...) seems OK.

Then we only have remaining naming convention updates.

Btw: running coder over the code shows me there's a lot to do.

These patches will also change a LOT (~100) of lines so we shouldn't hesitate to fix further formattings that as soon as the bugfix was applied.