This module allows you to construct a path alias based on taxonomy terms or menu items.

When you click on an item in the dropdown box it takes the term or menu item and translate it to an url-friendly string and append it to the url. This should give you the freedom of manually picking your url path and keep you from misspelling the url.

From the admin/config/search/path/settings page you can select the vocabularies and menus that you wan't to be available from the node edit page.

You can take advantage from the jQuery Chosen library (http://harvesthq.github.io/chosen/) downloaded from https://github.com/harvesthq/chosen/releases/download/v1.2.0/chosen_v1.2.... This library makes you able to search for a specific term or menu item. Extract the downloaded library to sites/all/libraries/chosen.
project page
https://www.drupal.org/sandbox/tlyngej/2344561
Git
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tlyngej/2344561.git path_alias_picker
Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxtlyngej2344561git

Comments

pkamerakodi’s picture

Status: Needs review » Needs work

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
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/README.md
No: Missing readme file
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
No. If "no", list security issues identified.
Coding style & Drupal API usage

1) Implement hook_requirments to check for the presence of required chosen library

2) jQuery("select.path-alias-picker-list").change(function () { use $ instead of jquery since you have create a local scope variable for jQuery

3) since you are using form alter to add js file better to use form['#attached']

4) I dont think you need this filter_xss($chosen_download_link)

Prajwal

tlyngej’s picture

Status: Needs work » Needs review

Updated code according to review report at #1

PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxtlyngej2344561git

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.

tlyngej’s picture

Status: Needs work » Needs review

Updated code according to #3

FILE: /var/www/drupal-7-pareview/pareview_temp/README.md
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
35 | WARNING | Line exceeds 80 characters; contains 105 characters
--------------------------------------------------------------------------------

This line contains a link, causing it to exceed the limit of 80 characters.

gaurav.pahuja’s picture

Correct link to clone code is

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/tlyngej/2344561.git path_alias_picker

naveenvalecha’s picture

Issue summary: View changes

Updated issue summary.

naveenvalecha’s picture

Issue summary: View changes
gaurav.pahuja’s picture

Status: Needs review » Needs work

Thanks for your contribution.

Can you please let me know if there is there any dependency on Jquery Chosen library?
And if yes, then hook_requirement should be modified to set severity of REQUIREMENT_ERROR instead of REQUIREMENT_INFO.

I enabled this module without Chosen library and it doesn't show me any error.

naveenvalecha’s picture

Hi @tlyngej,
First thanks for the contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxtlyngej2344561git Please fix these as addressed here.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
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/README.md
No: Follows the guidelines for in-project documentation and the README Template.Please add the instructions of keeping the library with its folder name in the libraries.
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
  1. (+) Remove the version = 7.x-1.0 from the module info file it is autmatically added by the drupal.org
  2. The configure link set in the info file does not seems to not good as it is set. It is pointing to admin/config/search/path Please change the path from admin/config/search/path/settings#edit-path-alias-picker to admin/config/search/path/settings
  3. Path alias picker container is not showing fine on the node edit page under the URL path settings section for the terms.
  4. Fix the typo in path_alias_picker.module file on line 31.
  5. Also fix the suggestion as addressed in #8

This review uses the Project Application Review Template.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

tlyngej’s picture

#8
Can you please let me know if there is there any dependency on Jquery Chosen library?

No, Chosen is not a dependency. You can install it, and get some extra features, as the README.md states, but it's only nice to have.

tlyngej’s picture

#9

Best practice issues identified by pareview.sh / drupalcs / coder. http://pareview.sh/pareview/httpgitdrupalorgsandboxtlyngej2344561git Please fix these as addressed here.

As I wrote earlier, this is caused b a long link. And the reason for the best practice is not being followed here is that I want to keep the readability and consistency of the list items more than trying to satisfy the automated test. I compared to how others made lists, and this seemed to be the way to do it.

Please add the instructions of keeping the library with its folder name in the libraries.

Yup, good point. Done.

Remove the version = 7.x-1.0 from the module info file it is autmatically added by the drupal.org

Fixed

The configure link set in the info file does not seems to not good as it is set. It is pointing to admin/config/search/path Please change the path from admin/config/search/path/settings#edit-path-alias-picker to admin/config/search/path/settings

The configuration link worked fine, but I fixed it to a link with no anchor in it anyway. Just, thought it would be nice to direct the user to the relevant part of the page. :-)

Path alias picker container is not showing fine on the node edit page under the URL path settings section for the terms.

Not sure what you mean by this. I've made a demo of how the module works here. If this matches what you see, this is how it's designed to work. Not that it's perfect, I just havn't done much styling of it yet. http://youtu.be/wxRBSP6aonQ

Fix the typo in path_alias_picker.module file on line 31.

Fixed

Also fix the suggestion as addressed in #8

Answered in #10

tlyngej’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Status: Needs review » Needs work

@tlyngej,
Please move the instructions of adding the library under the installation section in Readme.md because its the part of the installation.
Ok its fine if the jquery library link is long in the Readme.
Rest of the things seems good.

Please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

tlyngej’s picture

Status: Needs work » Needs review

Installation instructions moved.

naveenvalecha’s picture

Now, Please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

markconroy’s picture

Hi Tommy,

This looks like a great module. Three items I have discovered (though this might be part of the design)

1) When I edit a node that has a URL set via this module, and I then wish to change the URL, I would expect to be able to place the cursor where I want it and the new token would be inserted there. What happens is the new token is inserted at the end of the URL.

2) If I change a term on the taxonomy page, the URL is not updated. It probably shouldn't be, but you might want to add this note in as part of the documentation.

3) If I edit a node that has a URL set via this module, then add a new term/menu item to the URL, then move that new term/menu item to a different part of the URL (as in replace a term with the new one), I get a page not found error and can only re-save the page by going to admin/content to edit the node again.

Like I say, these may be part of the design. If so, it would be nice to have that documented.

tlyngej’s picture

@mark

Great ideas. Thanks for testing.

But for now, things are working pretty much as designed. Not that there aren't room for improvement. But I rather have it, as it is, right, before I start changing it. :-)

I'll add your suggestions to the issue queue for the project.

Thanks again.

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

@tommy

Thanks for the reply. Looking forward to using this on a number of projects once it has a full release status.

stella’s picture

Hi Tommy,

Great module, just a few small changes recommended:

1. Line 27 of the install file goes beyond the 80 characters.
2. Line 27 also embeds url variables in the t() function through the use of !tokens where the variable inserted are defined like "$libraries_download_link = l($t('here'), 'https://www.drupal.org/project/libraries');" This is secure, so that's not the issue. The issue is that there is no context provided to the translator when they are translating the word "here". Instead you should embed the anchor html markup and the word "here" within the full string being translated and then use the @url type token to insert the url, e.g. use $result = t('Click <a href="@url">here</a>', array('@url' => $url))); rather than $result = t('Click !here', array('!here' => l(t('here'), $url))); See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7 for more details
3. Lines 45 and 70 of the module file have untranslated "Choose a term" and "Choose a menu item" strings. These should be run through the t() function.
4. Lines 56 and 81 have a similar issue to point 2 above, in this case the words "terms" and "menu items" are out of context, and potentially the wrong order depending on the language being translated to. So for example '#title' => check_plain($taxonomy->name) . ' ' . t('terms'), should become '#title' => t('!name terms', array('!name' => check_plain($taxonomy->name))), but actually since you are calling check_plain() on the term name too you could probably simplify it as '#title' => t('@name terms', array('@name' => $taxonomy->name))

stella’s picture

Status: Reviewed & tested by the community » Needs work
tlyngej’s picture

Status: Needs work » Needs review

@Stella

Thanks for spending time on this.

I fixed all 4 issues.

stella’s picture

Status: Needs review » Reviewed & tested by the community

Looks much better!

naveenvalecha’s picture

As git administers are currently quite busy with all the project applications.Take a review bonus. by reviewing other project applications. This will put yourself on the high priority list, then they will take a look at your project right away :-)

er.pushpinderrana’s picture

Assigned: Unassigned » er.pushpinderrana
er.pushpinderrana’s picture

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, Below issue found.

Review of the 7.x-1.x branch (commit fc28d0a):

FILE: /var/www/drupal-7-pareview/pareview_temp/README.md
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
37 | WARNING | Line exceeds 80 characters; contains 105 characters
--------------------------------------------------------------------------------

Manual Review

You should also really have a hook_help() with some basic info about the module.

path_alias_picker_requirements(): In general, url() is preferred to make an absolute URL.

But that are not application blockers, assigning to mpdonadio for a second look if he has time.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +PAReview: security

Do you ever set path_alias_picker_taxonomies and path_alias_picker_menus? You variable_get them. but I am not seeing how the get set.

(*) Pretty sure you have an access bypass problem. In path_alias_picker_form_node_form_alter(), you call menu_load_links() which does not do any access checking. So, users may see node titles and other menu entries that that may not have access to, which is a security problem. You need to call menu_link_load() on each item and then check access. This step will also translate the menu title.

tlyngej’s picture

The two variables are set here and here

You are indeed right. The titles I used was not access checked. They are now per my latest patch.

Thanks!

tlyngej’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll look at the later today.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Reviewed & tested by the community

(+) You need to have a hook_uninstall() to variable_del() the variables you create.

My blocking issue was taken care of.

mpdonadio’s picture

Status: Reviewed & tested by the community » Fixed

This was RTBC by an admin before my review, so...

Thanks for your contribution, tlyngej!

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:

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.

Status: Fixed » Closed (fixed)

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