[D7] Pagemanager path aliases

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

The automated review checks out cleanly! (http://pareview.sh/pareview/httpgitdrupalorgsandboxsandergo902221921git) So does my manual test. This module is useful! But I think you could add more details to the description field about trailing and leading slashes. It's unclear if you are supposed to include them and it would be a nice consistency with other path declaration forms.

Also, it's a small detail but you may want to consider adding a space in the module name and everywhere else it appears so that it's 'page manager path alias' to match the 'page manager' syntax which is two words. The two words keeps things consistent.

I'm leaving this on needs review for a second set of eyes to look at it and perhaps for you to tweak those small details, but otherwise nice job!

1-I have been constantly getting warning (screen shot attached), may be you have to use isset on line 15
2-change git link to now git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sandergo90/2221921.git page_manager_path_aliases

Usability Review

I'm not very familiar with page manager or panels, but I did my best to try and use this module.
I followed the example in your readme and created a contact page, an about us page and an faq page. After placing faq under both contact and about us in the menu I could see that the active trail showed both faq pages as active.
I then went to page manager and added a custom page for my faq page. I tried added the page path as /faq but that resulted in an error so I went with /node/52. The Variant type field was a bit confusing as I didn't want to create a panel or an HTTP response code, I just wanted a second url alias, but went for panel.
I then went to the Path aliases panel and set my aliases as faq, faq1 and faq2 and clicked update. This resulted in the following errors:

Notice: Undefined property: stdClass::$src in page_manager_path_aliases_settings_form_validate() (line 139 of /var/www/test/htdocs/sites/all/modules/page_manager_path_aliases/page_manager_path_aliases.module).
That path is currently assigned to be an alias for . This system cannot override existing aliases.
Warning: explode() expects parameter 2 to be string, object given in page_manager_page_get_named_arguments() (line 500 of /var/www/test/htdocs/sites/all/modules/ctools/page_manager/plugins/tasks/page.inc).
Warning: Invalid argument supplied for foreach() in page_manager_page_get_named_arguments() (line 501 of /var/www/test/htdocs/sites/all/modules/ctools/page_manager/plugins/tasks/page.inc).

Once of these is generated by your module, but the notice and warning's should be fixed.

After removing faq from the list the errors disappeared and I could save my changes.

At this point neither faq1 or faq2 actually pointed to my faq page, however I think this could just be down to my incompetence with the page manager and panels modules (it seems much easier to achieve everything they do though code than using their UI!)

Code Review

All looks good apart from:

• page_manager_path_aliases.module ln 63 - I don't think there should be an ampersand then a space before $form_state.

Hi Sander,

Thanks for the explanation of how to use panels - was getting rather confused before!

I've now managed to create a panel page, add some content to it, and create multiple aliases for the same page. I then added each alias to different menu items and the correct trail was shown on each of them so the module is doing what it says it's meant to.

I tried to recreate the errors that I came across before and can confirm these are fixed.

I was able to add existing paths an a path alias, however this had no effect on that page. For example I added added "node/3" and didn't receive a warning, but also had no effect so I can't really see this being an issue.

There is 1 pareview error: page_manager_path_aliases.info - 7 | ERROR | Files must end in a single new line character.

And in page_manager_path_aliases.module one line 171 - should not be an ampersand before $form_state.

These are both minor issues so I'm happy to give RTBC status.

.info:
I'm not sure this actually does depend upon panels.

.module:
Don't use extra whitespace in a call to t(). This makes it difficult for translation. One long line is sufficient.

    '#description' => t("Enter one path per line. The path can't start with
     a leading slash or end with a trailing slash. You cannot have a dynamic
     path element after an optional path element. The first element in a path
     may not be dynamic. You cannot have a static path element after an optional
     path element."),

Alias validation doesn't account for language. See path_form_element_validate for an example. Why don't you use drupal_lookup_path?

Menu validation should be able to use drupal_valid_path?

Moving to needs work for the translation piece and the alias validation. The rest is highly encouraged though.

Moving to RTBC and waiting for another git administrator to give this a second set of eyes.

Assigning to myself for the second look when I can get to it. @heddn, feel free to assign to me for this in the future.

Automated Review

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Minor, btu fix when you can.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

So, this works by duplicating the router item for each alias? Why can't you use the Path API? Wouldn't this mean
that globalredirect wouldn't work for these aliases? This needs some commenting / documenting, as it looks like
these are cloned menu items and not path aliases. After reading the code, I think I get the issues, but it should
be made clear. If these really are cloned menu items, you may want to rethink the module name, as calling them
path aliases would be a little misleading.

You may want a permission to be able to set the path aliases.

What happens when you enable a disabled page that has aliases, and you have overlap?

The db_query() in page_manager_path_aliases_settings_form_validate() can be simplified as it looks like you
are only using one column. It may also be better to use menu_get_item() instead of the query, in case the menu
needs to be rebuilt (and, in general, it is preferable to program against the API and not the implementation whenever possible).

Will this work on panel variants, or just at the panel page level (my test site doesn't have any panel pages on it right now)?

I think calling menu_rebuild() instead of the variable set may be a better option.

I think this needs a little polishing before a proper release, but I am not seeing any blocking issues.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Here is the missing attachment.

Thanks for your contribution, sandergo90!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.