Please review Pagerer project for full project application.

Pagerer is a Drupal 7 module providing a collection of pager themes to enhance Drupal standard pager.

Project page: http://drupal.org/sandbox/mondrake/1410964

git: git clone http://git.drupal.org/sandbox/mondrake/1410964.git pagerer

Demo: http://mondrake.org/home/pagerer/example

Documentation: http://mondrake.org/home/node/4

Thanks,
mondrake

Reviewed project applications:

#1626870: Login Redirect, http://drupal.org/node/1626870#comment-6158664
#1553604: Burstn, http://drupal.org/node/1553604#comment-6171618
#1553456: Text Selection, http://drupal.org/node/1553456#comment-6175792

Comments

cbeier’s picture

cbeier commented
Status: Needs review » Needs work

Hi and welcome,

There are some coding style issues. You can use this online tool to detect the issues automatically: http://ventral.org/pareview/httpgitdrupalorgsandboxmondrake1410964git

* README.txt is missing, see the guidelines for in-project documentation.
* Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

The .module file must not be listed in the .info file (files[]).
Your module must not be defined as an dependency in the .info file.

pagerer.install:
- pagerer_install(): I think the menu_rebuild() call is not needed here.
- pagerer_uninstall(): Use the variable_del() function instead of the db_query() function. You don't need to clear the cache separately.

Regards, Christian

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Thanks cbeier for your time and helpful comments.

Did some changes:

a) ventral now indicates no issues
b) added README.txt
c) created branch 7.x-1.x and deprecated branch master (GIT)
d) no files[] in .info file
e) removed menu_rebuild() from pagerer_install()

Then:

f) changed pagerer_uninstall() to use variable_del(). Still, a select db_query() is there to list all possible variables beginning with 'pagerer'. Even if now there is only one configuration variable, 'pagerer', still there may be more in future developments.
g) dependency to 'pagerer' module is needed in 'pagerer_example.info' as 'pagerer_example' actually depends on 'pagerer'

Many thanks again, looking forward to getting more comments,
mondrake

orakili’s picture

orakili commented
Status: Needs review » Needs work

Hello,

First thank you for the module. The code is well-structured, easy to read and understand.

Some comments after testing the module and reading the code:

- pagerer.module (lines 1668 to 1677). You could replace the block checking clean url by url(request_path(), array('query' => $variables['parameters']));.
- pagerer.module (line 1778). Similarly, you may want to replace $_GET['q'] by request_path() or current_path().

- pagerer.js line 9. You set Drupal.pagerer. But it's actually not used.
- pagerer.js line 15. "$(document).ready" is not needed and should not be used as it's handled by Drupal.behaviors already.

The module doesn't work properly with tao-based themes as tao overrides theme_pager_link and so on. Indeed those theme functions return an array containing suitable information to be used with "l" function instead of a marked up HTML link. I would suggest to test if what is returned by theme('pager_link') is an array and in this case use "l" or something similar to generate the appropriate link.

Except for the above, the module looks ready to go.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Hi orakili,

many thanks for your time spent on reviewing this module, and pointing to the issues.

- pagerer.module (lines 1668 to 1677). You could replace the block checking clean url by url(request_path(), array('query' => $variables['parameters']));.

I changed the code to use url() and base_path(). I need to pass separately the base path and the rest of the url to the js code, so I could not use url() only. The reason to pass base_path and the rest separately is because of the different logic for page relocation in case of a 'standard' page (in which case full url is needed), vs. pages rendered on the admin overlay (these use bbq and require url less the initial base_path).

- pagerer.module (line 1778). Similarly, you may want to replace $_GET['q'] by request_path() or current_path().

The entire _pagerer_override_theme_pager_link() function is an exact copy of the Drupal core theme_pager_link() function that I override in the module, but for a single statement that I had to change as I believe is a bug, see #1588138: pager_query_add_page() [in D7, theme_pager_link()] overrides parameters passed programmatically. Ideally, if core is fixed, then I could drop the entire function. Given that, if it is not a problem, I'd prefer to keep my copy as aligned as possible with the Drupal core version.

- pagerer.js line 9. You set Drupal.pagerer. But it's actually not used.
- pagerer.js line 15. "$(document).ready" is not needed and should not be used as it's handled by Drupal.behaviors already.

Fixed - thanks.

The module doesn't work properly with tao-based themes as tao overrides theme_pager_link and so on. Indeed those theme functions return an array containing suitable information to be used with "l" function instead of a marked up HTML link. I would suggest to test if what is returned by theme('pager_link') is an array and in this case use "l" or something similar to generate the appropriate link.

I introduced a helper function that checks if the output of a theme_pager_xxxx() function is a valid input array for theme_links() (the format used by Tao), and in that case, renders it to HTML through the theme itself. I tested a bit with an out-of-the-box Tao 7.x-3.0-beta4, and the pagerer_example page, which calls pagerer_xxxxx themes directly, seems ok (apart from styling). However, since Tao overrides theme_pager() at the theme level, there is no way to enforce a change of the Tao pager from the theme's one to Pagerer's one (i.e. the setting in admin is ineffective).

Thanks again, looking forward to further comments,
mondrake

sylvain lecoy’s picture

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

Manual review:

The .module file is really massive. However code looks really good to me.

The JS part has been fixed, I think I can RTBC you.

orakili’s picture

orakili commented

Thanks for the modifications Mondrake.

As for the helper _pagerer_resolve_link_data. If you replace the call to theme('links') with $link = l($link['title'], $link['href'], $link);, the problem is solved. Teme_links is well suited for an array of links but not for individual ones as it creates an unordered list. Inside the code of theme_links, it's mentioned that links (if link['href'] is set) are suitable to be passed to the "l" function. The code above is from there. In tao, once each link array has been generated with theme_pager_xxxx(), then the list of links it passed to theme_links not individually.

That's only thing.

Thanks again for the module.

RTBC as well.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

mondrake’s picture

mondrake
Location 🇮🇹
commented

@orakili - your comment at #6 - done and pushed to repo. Many thanks for the hint!

@klausi - I will give a try. Is reviewing other applicants' modules the only way to get the bonus, or is there any other community work that can help? I am asking since I made some effort on #1649352: Proposal to contribute to Textimage 7.x-2.x . But maybe this is off topic.

cheers
mondrake

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

typo

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +PAreview: review bonus

adding review bonus

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

manual review:

  1. make sure that README.txt is UTF-8 encoded, I see some strange characters (Non-ISO extended-ASCII English text).
  2. pagerer_theme(): indentation errors for the array key => value assignments.
  3. pagerer_pane_settings_form_validate(): empty function, so remove it.

But that are just minor issues, so ...

Thanks for your contribution, mondrake! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, 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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks klausi and thanks a lot to all the reviewers.

Cheers
mondrake

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added reviewed projects