Favicon By Path

Please make your project follows Drupal coding standards first because it still has few errors. See http://ventral.org/pareview/httpgitdrupalorgsandboxaptorian1826336git for more details.

Hi @aptorian,

Automatic Reviews:
No Problems Found about code standards according to Coder Review module.

Manual Reviews:
I found some issues that should be solved.

1) I went to the settings page of Favicon By Path module, then i tryied to insert a second favicon with one new path. How ever when i tryied to save changes it didn't work. A blank page is displayed to me whenever i try to save changes after added a new favicon with paths.

2) When i access a page that are not configured by your module it displays a notice: "Notice: Undefined variable: matched in favicon_by_path_preprocess_page()". It happens because you use a $matched variable that were not assigned a initial value.

It is the line error:

// If we found a match, stop checking.
if ($matched) {
  break;
}

To solve it you should declare this variable in the beggining of your function favicon_by_path_preprocess_page and assign a FALSE as value exactly as i've done in this example:

function favicon_by_path_preprocess_page(&$vars) {
  $matched = FALSE;
  //....
}

I attached a image related to this error.

3) Replacement Patterns fieldset in settings page is empty. It happens because it is mandatory to have installed Token module to show his contents. So you should add this dependency in favicon_by_path.info.

It is all. I appreciated this module.

In the t() function, you don't have to call check_plain manually so long as you use the right placeholder token.

For example, in the admin.inc file on line 206

drupal_set_message(t("There was an error deleting favicon %id.", array('%id' => check_plain($form_state['values']['id']))));

Have a look at format_string for more details http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/forma...

In this case, if you change the code to read

drupal_set_message(t("There was an error deleting favicon %id.", array('%id' => $form_state['values']['id'])));

The format_string function will trigger drupal_placeholder (http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupa...) which runs check_plain already.

Have a look at anywhere you output text and adjust accordingly.

In your exported configurations, you seem to be using the return character as a delimiter for the paths. I would suggest instead either using a serialized array of values, or some kind of visual delimiter.

If you go the delimiter route, I would be careful to ensure you select a delimiter which isn't valid in a URL path nor in your replacement patterns so you don't get any false positives during parsing.

One last recommendation. If your schema, instead of using a serial id value as your primary identifier, I'd suggest using a machine name or other unique value. It helps to avoid conflicts when porting exported configuration between sites.

Have a look at the machine_name http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht... form api option.

Hi @aptorian!

You asked me if that bug still happens. I installed your last version and it is working properly here!

Good job!

Please have a look few observation from technical point of view,

Use of drupal_static should always,

 $elements = &drupal_static('drupal_add_html_head');
        if (!isset($elements)) {
          foreach (element_children($elements) as $id) {
            if (isset($elements[$id]['#attributes']['rel']) &&
                ($elements[$id]['#attributes']['rel'] == 'shortcut icon' ||
                 $elements[$id]['#attributes']['rel'] == 'icon')
            ) {
              unset($elements[$id]);
            }
          }
       }

as drupal_static store the central static storage so expensive logic should be always kept into it. It basically improves the performance of the execution of snippet as it try to go-through elements of head section and unset existing favicon..

favicon_by_path_load_all() & favicon_by_path_load($favicon) both can be merged into a single function.
depending on the parameter we can make condition to execute the logic!

Sorry I couldn't understand one point why I can't use hook_html_head_alter() for current_path only?
current_path() returns $_GET only! better to use alter hook in your case instead of preprocess_page hook
& you can avoid the way you use drupal_static... other wise execution become so expensive!

If you look at hook_html_head_alter, it ends up calling drupal_static anyways so no big savings on performance there.

There's a bug with dpm and current_path in hook_html_head_alter (gives the wrong value) which probably explains why things weren't working/confusing for aptorian in the hook_html_head_alter.

In hook_html_head_alter I don't think he'd have access to the proper tokens for path replacement

function favicon_by_path_preprocess_page(&$vars) {
  $favicons = favicon_by_path_load();

  // Array of objects to be used for token replacement.
  $data = array();
  if (isset($vars['user'])) {
    $data['user'] = $vars['user'];
  }
  if (isset($vars['node'])) {
    $data['node'] = $vars['node'];
  }

Personally, I'm ok to RTBC the module. We can leave the question of performance to the issue queue for the module. I don't think that's a blocker for the project application.

I'll switch this back to needs review, see if there's consensus for RTBC.

In hook_html_head_alter I don't think he'd have access to the proper tokens for path replacement
=> what do you mean by this?

if you please look into snippet,

foreach (element_children($elements) as $id) {
            if (isset($elements[$id]['#attributes']['rel']) &&
                ($elements[$id]['#attributes']['rel'] == 'shortcut icon' ||
                 $elements[$id]['#attributes']['rel'] == 'icon')
            ) {
              unset($elements[$id]);
            }
          }

tries to unset existing rel tag by above loop & condition after that inject the own
tag drupal_add_html_head_link...
that thing can be achieved by http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct... using alter please have a look the examples in link!

NOW it's RTBC to me! if any one have new suggestion please let us suggest!

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 :-)

Thanks for your contribution, Christopher!

I updated your account to let you 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 get 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.