CVS edit link for mdorman

I am looking to contribute a new performance module I have been working on. The new module will allow less technical site owners to implement CDN technology as a service to reduce the need for them to maintain separate systems. While similar to the cdn module, it will not duplicate any of it's functionality and should work well together.

I have been using Drupal for a number of years and build very high traffic Drupal sites during the day. It is my hope that this module helps the low and medium traffic sites decrease load times as well.

CommentFileSizeAuthor
#9 cmscdn.zip4.11 KBmdorman
#5 cmscdn.zip2.87 KBmdorman
#1 cmscdn.zip8.5 KBmdorman

Comments

mdorman’s picture

mdorman commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new cmscdn.zip8.5 KB
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

mdorman’s picture

mdorman commented
Status: Needs work » Needs review

Module Description:

This module allows for multiple site owners to utilize a domain and CDN for all core JavaScript files. By serving these shared files from a single domain and CDN end users will gain significant caching as they go from one Drupal site to another. In addition, the shared domain will serve the files from a CDN to geographically locate the responses from
a server nearby.

As an example rather than serving the /misc/jquery.js file from your local server, or even a private CDN domain after installing this module you're file will be served from:
http://ajax.googleapis.com/ajax/libs/jquery/1.2.6/jquery.min.js

Other core files, that are specific to Drupal will be available based on your version of Drupal which is determined by the module. For example the /misc/drupal.js file will now be served here if you were using version 6.17:
http://j.cmscdn.com/drupal/6.17/misc/drupal.js

This module is not a replacement for the http://drupal.org/project/cdn module, however can work along side that one if you feel it is necessary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Thank you for your reply.

mdorman’s picture

mdorman commented
StatusFileSize
new cmscdn.zip2.87 KB

I'm uploading an updated module based on feedback I've seen in other module submissions. This version is a fully functional version with the correct domain and works with 6.16, 6.17, 6.18. I will include all other 6.X versions before final release.

  • Removed licences.txt
  • Fixed a few coding standards found by coder
  • Updated CDN domain
  • Modified search/replace to find all /misc/*.js files

Looking forward to the feedback.

-Matt

mdorman’s picture

mdorman commented

I wanted to add a little more background to this issue to see if I can get some feedback of whether or not this module has potential to get approved. I'd like to continue development on porting this to D7 and possibly even D5, but without a project page with an issue list it's hard to get the necessary testing done.

This module will make performance issues relating to aggregation far less important as it pulls all core JS, and eventually CSS out of local aggregation and allows them to be served externally. Here are the related D7 issues:

http://drupal.org/node/769226
http://drupal.org/node/630100
http://drupal.org/node/721400
http://drupal.org/node/101227

meba’s picture

meba commented

From security point, there is nothing wrong with the module. From usability point, I don't really see a point of it. It redirects your misc/*.js files to j.cmscdn.net without any possibility to change that domain. What happens when module author decides not to run this domain anymore? Sites will be broken.

mdorman’s picture

mdorman commented

Very valid point about if j.cmscdn.net goes away, your site goes down. However, the same can be said for any other module that connects you to a service. The goal of this module is to allow less technical site owners and those hosting in shared environments to have the benefits of serving Core files from an external CDN to gain end user and server performance gains.

That being said, having manual as well as automated fail overs back to local is a good recommendation. The support of the free service will later be supplemented by paid services for more specialized/unique caching requirements.

I hope this alleviates some of your concerns, this will also be listed on the project page as a note.

mdorman’s picture

mdorman commented
StatusFileSize
new cmscdn.zip4.11 KB

Uploading the latest version of the module, which adds the following:

  • Admin Settings page for switching domain
  • Better support for aggregation
    • Now serves aggregated misc.min.js file when aggregation turned on
    • Reduced number of files being aggregated on the server by up to 80KB
  • Dynamically finds all core js files under /misc and /modules/* now
  • Caches swfupload libraries when included by image_fupload module

One known issue is an incompatibility with jquery_update.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Fixed
  • The points reported in this review are not in order or importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. The version line needs to be removed from the .info file. 
  2. /**
 * Implementation of hook_install().
 */
function cmscdn_install() {
  // Do Nothing
}

    When a hook is not necessary, it should not be declared.
    Hook implementation comments should be like the following one:

    /**
 * Implements hook_menu().
 */

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: Miscellaneous » new project application
Assigned: Unassigned » avpaderno
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented