Follow-up for the issue I created: https://github.com/MaxCDN/bootstrap-cdn/issues/460

Instead of statically providing the supported CDN versions for both Bootstrap and Bootswatch, we should instead populate them dynamically via the jsdelivr API.

CommentFileSizeAuthor
#20 provide_a_more_robust-2450757-20.patch99.46 KBneardark
#9 bootstrap-provide-a-more-robust-2450757-9.patch78.63 KBmarkhalliwell
#4 bootstrap-populate-available-2450757-3.patch16.29 KBmarkhalliwell
#3 bootstrap-populate-available-2450757-3.patch5.37 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

waverate’s picture

waverate CreditAttribution: waverate commented

Mark,

This looks good for Bootstrap:

http://maxcdn.bootstrapcdn.com/data/bootstrapcdn.json

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented

That's just for Bootstrap. In the GH issue I linked, I learned that file is automatically generated from http://api.jsdelivr.com/v1/bootstrap/libraries. Considering that this JSON data has both Bootstrap and Bootswatch, I think it'd be better to use that one directly.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
FileSize
bootstrap-populate-available-2450757-3.patch5.37 KB

Initial WIP (non-working) patch of dynamic CDN

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
FileSize
bootstrap-populate-available-2450757-3.patch16.29 KB

Sigh... it didn't add the new file... trying again...

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
Title: Populate available CDN versions via jsdelivr API » Provide a more robust CDN solution
waverate’s picture

waverate CreditAttribution: waverate commented

Mark,

Against latest 7.x-3.x-dev, I get a WSOD on the settings page. From log:

Notice: Use of undefined constant BOOTSTRAP_MAJOR_VERSION - assumed 'BOOTSTRAP_MAJOR_VERSION' in _bootstrap_cdn_api() (line 113 of sites/all/themes/bootstrap/includes/cdn.inc).

Using php 5.4.38

Christopher Riley’s picture

Christopher Riley CreditAttribution: Christopher Riley commented

I ran into the same issue. It appears that it is not reading the define out of template.php. I added the same define to the top of includes/cdn.php and it works and does not give a already defined message. I added a line to the top of template.php saying it was loaded and another at the top of cdn.php and I see the message about cdn.php but not template.php.

How does the loading order work with a subtheme? Could this be a subtheme related issue? For the time being I moved the define over to cdn.php and all is working but would love to see this cleaned up and implemented.

Thanks in advance.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented

WIP (non-working) patch

I would have thought this was pretty self-explanatory...

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
FileSize
bootstrap-provide-a-more-robust-2450757-9.patch78.63 KB

Final patch

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Assigned: Unassigned » neardark
Status: Active » Needs review

Ok. I think this is "done" and ready for review/testing.

@neardark, please advise. Doesn't have to be ported to 8.x just yet. Just need as many eyes as possible on this.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
neardark’s picture

neardark CreditAttribution: neardark commented

Only two minor minor items noted below. I need to re-review when my mind is clear, but this looks great. I did install, and it worked well. 

  1. +++ b/includes/cdn.inc
@@ -0,0 +1,490 @@
+ *   $provider is not set. If $provider is set and exists, it's individual

+++ b/includes/common.inc
@@ -104,6 +173,42 @@ function bootstrap_include($theme, $path) {
+  if (isset($deprecated[$prefix . $name]) && ($setting = theme_get_setting($deprecated[$prefix . $name], $theme))) {

@@ -900,7 +1005,7 @@ function _bootstrap_tooltip_description($description) {
 }

    it's should be its 

  2. +++ b/includes/cdn.inc
@@ -0,0 +1,490 @@
+            '!drupal_http_request' => 'https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_http_request/7',

    %21 should be a forward slash I believe.

waverate’s picture

waverate CreditAttribution: waverate commented

Patch #9 with #12 changes works.

  • markcarver committed a6c45bc on 7.x-3.x 
    Issue #2450757 by markcarver: Provide a more robust CDN solution
markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented
Version: 7.x-3.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)

Ok, I addressed #12.1. #12.2 is correct, API doesn't recognize a forward slash there.

Moving this to 8.x-3.x for port and more review. If there's any further issues found with this work, please open a new issue to address.

I'll be creating a change record for this issue.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell at Tag1 Consulting commented

Published change record: https://www.drupal.org/node/2452617

  • markcarver committed 31afd0c on 7.x-3.x 
    Issue #2450757 by markcarver: Provide a more robust CDN solution

Minor...

  • markcarver committed a6722d9 on 7.x-3.x 
    Issue #2450757 by markcarver: Provide a more robust CDN solution

Fix...

  • markcarver committed 1427c5f on 7.x-3.x 
    Issue #2450757 by markcarver: Provide a more robust CDN solution

Fixed...
neardark’s picture

neardark CreditAttribution: neardark commented
Assigned: neardark » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
provide_a_more_robust-2450757-20.patch99.46 KB

  • neardark committed 1a8dedb on 8.x-3.x 
    Issue #2450757 by markcarver, neardark: Provide a more robust CDN...
  • neardark committed 961a3f6 on 8.x-3.x
    Revert "Issue #2450757 by markcarver, neardark: Provide a more robust...
  • neardark committed cf2dccc on 8.x-3.x
    Issue #2450757 by markcarver, neardark: Provide a more robust CDN...
neardark’s picture

neardark CreditAttribution: neardark commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

Pol’s picture

Pol
the/that
Location Brussels
CreditAttribution: Pol as a volunteer commented

Hi all,

I made a Libraries API integration module with CDN, you can find it here: https://www.drupal.org/project/libraries_cdn
It might certainly help you managing your CDN in a easy way.