[D7] Ajax Regions

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

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.

Hi Feint,
Nice module,it works as you have written but some points in code review.
1- You are adding javascript in hook_init and that javascript will add on all pages (Even on pages where it doesn't need).
2- You are using eval in javascript which can be a security concern and its very bad practice.
Thanks!

Hi FeintEars,

I think this is a great idea for a module and mostly found it pretty easy to use.

For testing I created a load of pages that link to each other that were in the content region and gave them all links to each other with a class of .ajax-link.

After this setting the link selector was obvious to me however the "Ajax-regions" field wasn't quite so clear. I didn't understand why I could just put the CSS selector or the region I wanted replaced in the same way the #wrapper property is used when using ajax as part of the Drupal form API. Can this field either be simplified, or explained more fully to tell users what the roles of the key are value (in the object) are.

I found the Update document title feature didn't always work. For example:

Page 1, page title: Decet Facilisi, doc title: Decet Facilisi
Page 2, page title: Pertineo Uxor, doc title: Decet Facilisi
Page 3, page title: Blandit, doc title: Blandit
Page 4, page title: Bene Valetudo Voco, doc title: Blandit
Page 5: page title: Decet Facilisi, doc title: Decet Facilisi

I then disabled this feature by unchecking the box and saving the settings, however upon navigating my pages I found that the document title was still getting updated.

Looking into this further I've found that only every other link is loaded via ajax. It seems that when content is loaded via ajax with ajax enabled links in the click event is not getting bound to the link.

To try and test the "Set loading-indicator" feature I created a link to an external slow site, but when I clicked the link I did not see a loading indicator appear, and also the site did not get loaded into the region. Should it work with external links? If it doesn't think I think this should be mentioned in the documentation.

Because of the bug where click events are not bound to link loaded via ajax disabaling the "Update current address" only worked every second link.

I had a play with "Update active links" feature but could not get it to do anything. I can see in your readme file that is says "setting to new link class = "active" (this class is used
in menu of the theme appearance Bartik)" but did not actually see any links that were given the active class. I think maybe I don't quite understand what is meant to happen here.

I did a quick review and found one area where you can do some adjustments:

On hook_init you are loading javascript using drupal_add_js, You dont wan't to run 'drupal_add_js' on drush commands. Therefore you can use it on the "hook_page_build()".

I added a clone link into your description as will make it easier for others to clone your project.

Hi FeintEars,

• The document title was not updated after loading new content into the ajax region.
• In ajax_region.admin.inc using the #attached property is preferable to using drupal_add_js(). See here for an explanation.

I'm still not really understanding the logic around the Ajax-regions field...

• What is the purpose of being able to pass in multiple regions?
• Why does each region need a name? Wouldn't just a selector be sufficient?

I tried using this value: {"header": "#header", "content": ".region-content", "footer": "#footer"}

When I clicked a link the header disappeared, the content was loaded into .region-content and nothing happened to the footer.

I think this need to be better documented as this is the second time I've had a play with the module and I just can't grasp the concept.

Can we work though an example? I think that will be the easiest way to fully understand it...

Lets say I have a link on a page, and upon clicking that link I want to load the content specified by the href of the link into the content region, and I also want to load a new block into the sidebar, and replace a block in the footer region.

How to I specify which blocks will be loaded into the sidebar and the footer?

How can you set an href for a block?

Do you mean a link inside the block? What if the block doesn't contain any links?

Duplication

This sounds like a feature that should live in the existing ajax_regions project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the ajax_regions issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Give it a week, then go for the abandoned process. The current maintainer might be on vacation or swamped with a project but a week should give enough time.

I hope this "Catch 22"-like situation is just a simple misunderstanding.

From the look of, it the "project ownership" team is not aware of that taking over an abandoned project is one of the things the reviewers here ask applicants to do, and that this is legitimate one route towards permission to create full projects.

I've posted a comment where I try to explain the situation. It may help if heddn also commented on this in the "project ownership" thread.

Cross-posting:

@FeintEars, once you become owner of the project, please provide an update to your project application as per https://www.drupal.org/node/894256#unsupported-module.

@FeintEars, and thanks for your patience in this process.

Cool, please update the issue summary once you adapted all your code and pushed it to the ajax_regions repository.

@FeintEars, if you are upgrading that module, then hook_update_N is in order if you wish to provide an upgrade path. Ideally you do provide an upgrade. re: javascript. You should use standard Drupal behaviors unless you find some good reason to the contrary.

Did I miss something?

Unfortunately, yes.

1. You've missed that you're not supposed to create a full (e.g. 7.x-2.0) release until the review process of your module has been fully completed.
   According to the Ajax Regions project page, you've gone ahead and created a full release 7.x-2.0 of the module without being fully through the review process (before passing code review and being given the RTBC + final review by a git vetted admin). In other words, you've short-circuited the entire review process and issued an official release of your module before it has been properly reviewed and checked for security. It shows an blatant ignorance on your part about how the release system on Drupal.org works. AFAIK, this is something that is not easily reversed, since there is no way to "take back" a full release once it is official.
2. Your issue summary links to your sandbox project and not to the 7.x-2.x development branch of Ajax Regions.
3. Your git clone command clones your sandbox and not the 7.x-2.x development branch that we're supposed to review.
4. The project page diffs shows that you've just replaced the entire content of the project page. You're supposed to retain history when you take over an abandoned project. The project page should still mention the 7.x-1.x branch and what it does. It should then go on and explain that as the new maintainer, you've discontinued support of that branch, and that the branch maintained in the future will be the 7.x-2.x branch. Also explain for current users how to migrate from 7.x-1.x to 7.x-2-x (or just say that there is no support for migration.
5. You should also make sure that the repo retains the history of the project. The code from the 7.x-1.x branch is in the repo, but that branch is untagged. There should be a "7.x-1.x" branch with its HEAD pointing to the final version of the "7.x-1.x" branch.

Should I close this issue?
https://www.drupal.org/node/1271484

That is up to you. It is a feature request. If you don't want to support that feature, then you close it. If you consider the suggested feature useful and may implement it later, when you can find the time, then you change the version to 7.x-2.x and keep it open as a placeholder.

At what stage should I rename php-functions from ajax_region_... to ajax_regions_...? (My sandbox-project was called ajax region, not ajax_regions)

Renaming the PHP functions is OK and have nothing to do with this.

What was wrong was that you created official tagged release with the tag 7.x-2.0.

You should not have created this tagged release before the project had passed review. You create official tagged releases at the end of the review process, not in the middle of it.

Try the following two commands in your local repo:

git tag -d 7.x-2.0
git push origin :7.x-2.0

I am not sure if the last of these (it is supposed to delete it at the origin) will work. If it fails, you probably need help from a webmaster to get rid of that release.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes. Project is taking over the namespace of abandoned project Ajax Regions.

Master Branch

No.

The default branch set up in the repo is the "master" branch in git. You should really be working from the 7.x-2.x version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

In addition, the legacy 7.x-1.x branch is missing from the repo. When taking over an abandoned project, it is important that the the new maintainer does not erase history by by erasing legacy branches. The legacy 7.x-1.x branch must be restored in the repo.

Do the following:

git checkout af13886
git checkout -b 7.x-1.x
git push origin 7.x-1.x
git checkout 7.x-2.x

Git Clone Command

No. Your git clone command clones your sandbox and not the 7.x-2.x development branch that we're supposed to review.

Licensing

No. Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it. See also: licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

Project page

No.

The project page diffs shows that you've just replaced the entire content of the project page. You're supposed to retain history when you take over an abandoned project. The project page should still mention the 7.x-1.x branch and what it does. It should then go on and explain that as the new maintainer, you've discontinued support of that branch, and that the branch maintained in the future will be the 7.x-2.x branch. Also explain for current users how to migrate from 7.x-1.x to 7.x-2-x.

Please read tips for a great project page for more about what the content you need to include on the project page.

README.txt

No: Please take a moment to make your README.txt follow the guidelines for in-project documentation and the README.txt Template. In particular, you need to describe how to configure the module, how to grant permissions, and the security implications of granting permissions. Also see notes under "Secure code" below.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Since the module is designed to let users granted permission to administer ajax_regions inject JavaScript via the admin interface, the project has obvious security implications. There is no way of blocking XSS exploits (inserted via the admin GUI) without crippling the module. Anyone granted the permission to administer ajax_regions can post inject arbitrary JavaScript (including XSS exploits). This role has severe security implicatons that need to made explicit several places (project page, README.txt and in the description of the permission grant).

Coding style & Drupal API usage

My notes:

• (*) There is a specific permission (administer ajax_regions) for access to the administration form for the module, but no hook_permission, so this permission cannot be granted.
• Using hook_help to provide instructions on top of a form is (AFAIK) unusual. Instead, you use the Form API with #markup to provide inline help with a form. I see no benefit in having the help text for a specific form in a separate hook. The most common use of hook_help to display a brief text about the module under the help meny in the admin bar. This is missing from this module.

The starred item (*) warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

now I should to fix it and create new release 2.0 without tag. Right?

Not right, I am afraid.

You need to understand the difference between branches and releases.

As long as the project is not ready for release, please do not try to create any releases.

What you do is to continue working on the 7.x-2.x branch, which eventually (when you're done) will become the 7.x-2.0 release.

You can check what branch you're working on with the command:

git status

it should say: On branch 7.x-2.x.

When you've done all the changes you want to do, you commit them to the local repo and then give the following command:

git push -u origin 7.x-2.x

That will push your changes to the HEAD of the 7.x-2.x branch of the remote repo, making your latest version available for everyone to review, just by cloning the HEAD of that branch using the exact git clone command you have put in your issue summary.

Right after pushing, you usually request a new review.

Did I miss it again?

No, that looks OK. I can toggle between 7.x-1.x and 7.x-2.x.

Can you try the following again:

git tag -d 7.x-2.0
git push origin :7.x-2.0

I believe that tag is no longer tied to a release on Drupal.org, so it should be deletable.

(You need to delete that tag, so that you can use it again later when the time is ripe for a release.)

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes. Project is taking over the namespace of abandoned project Ajax Regions.

Master Branch

No.

The default branch set up in the repo is the "7.x-1.x" branch in git, but should really be the 7.x-2.x branch. This is not a blocker tho'.

Git Clone Command

Yes, correct.

Licensing

Yes. LICENSE.txt removed. (Re: licensing requirements.)

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

Project page

No.

However, it now have information about both the legacy branch and the new branch, which is good. Layout and readability can be improved, and information about the legacy branch should be shortened (a brief mention that it exists and how it differs may be enough). It would be nice if it included more information about the project, as suggested in tips for a great project page, but this is not a blocker.

However, the module allows for XSS injection through the admin GUI, and this must be said clearly on the project page, so that site managers that are concerned about security knows that this module is not for them.

README.txt

Yes.

It now have a paragraph about the security implications of this project, which is good. As with the project page: Layout and readability can be improved, but not a blocker.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Since the module is designed to let users granted permission to administer ajax_regions inject JavaScript via the admin interface, the project has security implications. There is no way of blocking XSS exploits without crippling the module. Anyone granted the permission to administer ajax_regions can post inject arbitrary JavaScript (including XSS exploits). This role has severe security implicatons that need to made explicit several places. It is already in the README.txt, but should also be explicitly part of the description of the permission grant and on the project page.

Coding style & Drupal API usage

My notes:

• There is no hook_help(). It is good practice to display a brief help text under the help menu in the admin bar for every enabled module, but this is not a blocker.

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

Automated Review

FILE: ajax_regions/ajax_regions.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 90 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 90 | ERROR | Line indented incorrectly; expected 4 spaces, found 1
--------------------------------------------------------------------------------

PHP_CodeSniffer found at least one issue. 

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

(+) Yes: Follows the guidelines for in-project documentation and the README.txt Template. However, there are some weird characters in the README that should get fixed e.g.

�

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

• (+) js file: By default, Drupal passes settings into :attach. Use it is better performance to use it. Don't grab settings from the global namespace.
• (+) admin.inc file:
  Help text wrapped in t() within ajax_regions_admin should not include html tags. That kinda makes translation difficult.
• Minor finding
• (+) .install file:
  Don't adjust module weights. It is very brittle. Instead using hook_module_implements_alter() to push the module's hook to the head of the list (as necessary).

  function ajax_regions_install() {
    db_update('system')
      ->fields(array(
        'weight' => -100,
      ))
      ->condition("name", "ajax_regions")
      ->execute();

• .module file: It seems odd that you are inserting javascript inline rather than from a file. Why isn't is possible to load it from a single file? Especially when the after script doesn't have any logic in it anyway.
  

  function ajax_regions_page_build() {
    ...
      $before = variable_get('ajax_regions_before_loading');
      if ($before != '') {
        drupal_add_js($before, array('type' => 'inline', 'weight' => 99));
      }
  
      $after = variable_get('ajax_regions_after_loading');
      if ($after != '') {
        drupal_add_js($after, array('type' => 'inline', 'weight' => 100));
      }

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

Thanks for your contribution, FeintEars!

I updated your account so you can 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. And thanks for your extra patience in the process.

However, there are some weird characters in the README that should get fixed e.g.

The problem is that the README.txt uses depreciated ISO-8859-1 encoding rather than UTF-8, so the double arrows (») in the breadcrumbs show up as "�" on a system that expects UTF-8.

You can fix it by:

1. Converting the file from ISO-8859-1 with some character set conversion tool (e.g. iconv).
2. Replacing all non-ASCII characters with ASCII (s/»/>/g).

Uploaded README.txt with UTF-8 encoding (converted using iconv).

Use this instead (but remember to rename it back to README.txt).

Can I create a full release?

Yup!

Go ahead, and congratulations with completing the process.

last question: should/can I remove the master branch?

If you can, you should remove it. However, I suspect that it may be tied to some release in the past, so git will just give you an error if you try to remove it on the origin.

If you get an error, just leave it alone, it does no harm.

What you need to do now is to create a tagged release with the tag 7.x-2.0 (just like you did earlier, before the project was ready for release), push it, and then use the links at the bottom of the project page to make that release public. Expect around five minutes delay from creating a release until it appears on the project page.