Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend’s picture

Why is this critical?

oriol_e9g’s picture

Because we have to do this now or possible never in D7

marcvangend’s picture

Priority: Critical » Major

Currently, D7 is not broken because jQuery UI 1.8.5 is missing. According to http://drupal.org/node/45111, "Tasks and Feature Requests should never be marked 'critical'." This is major at best. If we keep marking stuff like this as critical, D7 will never be finished.

mfer’s picture

Category: feature » task
Priority: Major » Critical

I'm marking this as critical because it is considered a bad idea to launch a new version of Drupal with an outdated version of a library like this. Critical items are release stoppers. Releasing a version of Drupal (7.0 release) with a version of jQuery UI that has had bug fix releases that we did not incorporate is a bad idea.

When we have released Drupal in the past it has been with the latest jQuery releases. We need to continue that trend.

Bojhan’s picture

If you think its so critical supply a patch please.

mradcliffe’s picture

Just posting some reference material.

There have been many bug fixes with the five releases since 1.8.0 was patched for Drupal in #679036: Upgrade to jQuery UI 1.8. Many of these look important for UI developers to be aware of, but not security-related. Changelog links:

Jackinloadup’s picture

Will it be possible to upgrade jQuery UI minor releases after D7 is released?

drifter’s picture

Status: Active » Needs review
FileSize
444.59 KB

Well, here's an attempt. Replaced the jquery.ui files to their 1.8.5 equivalent, and updated system.module with the new version numbers.

mfer’s picture

@drifter I'll take a look at this. Thanks for the patch.

@jackinloadup I cannot say if core will update the version of jquery ui shipped. If not, there will be a contrib module to do so.

marcvangend’s picture

Do the automated tests cover javascript changes like this, or do we need to change this manually?

Jeff Burnz’s picture

@#7, maybe we'll need a jqueryui_update.module? Sub_scriben....

jrabeemer’s picture

This patch will go in if there's enough testing. Drupal isn't going to beta for 3-4 weeks so there's plenty of time to test.

drifter’s picture

Priority: Critical » Major

Also: not critical. Nothing is broken. Read through the JQuery UI changelogs and commit messages, couldn't find anything major.
Update modules will be provided in contrib anyway, as they have existed in the past.

Would be nice to include this, but definetly not critical.

mfer’s picture

Priority: Major » Critical
FileSize
447.47 KB

@drifter Critical issues are release blockers. Drupal 7 should not ship without being on the latest 1.8.x release. There are bugs in jQuery UI that may creep into sites we will build.

@marcvangend The automated tests cover the PHP handling of JavaScript but that's all. The way JavaScript interacts in a page, the animations, etc. are not tested. It is a product of the type of system we are using. So, JavaScript functionality is still manual for the time being.

@Jeff Burnz jQuery UI updates, after the D7 final release will happen through either the jquery_update module or the jquery module. We are still working on the plan for this in D7.

Now, on to the patch. There were a few more changes than just a simple swap of files.

  • The tops of the files need the $Id$ tag for CVS. I will be happy when this goes away.
  • The file misc/ui/images/ui-anim_basic_16x16_0.gif is not loger part of jquery ui.
  • There is a new file in misc/ui/jquery.ui.selectable.css.
  • The version in system_library and the CHANGELOG.txt still reads 1.8. This should read 1.8.5.

The attached patch makes these changes. I tested in Safari and Chrome where everything seemed to work.

Status: Needs review » Needs work

The last submitted patch, jquery-ui-1.8.5_916968_14.patch, failed testing.

ksenzee’s picture

Priority: Critical » Major

I know this feels like it should be a release blocker, but I honestly don't think it is. We can fix it anyway though.

Besides the patch not applying (because of a problem with jquery.ui.selectable.css), I'm seeing a couple more problems:

- @VERSION appears in several files instead of 1.8.5. This might be an upstream problem, but we should get it fixed before we add it to our codebase and cause all kinds of confusion.
- We're overwriting a lot of existing $Id$ strings that are already in CVS.

webchick’s picture

Yeah. This isn't a major version change in jQuery UI, so there's no reason we couldn't roll this out in 7.1 or 7.2. No reason for it to block release. Hopefully it's ready well before that becomes an issue though. ;P

webchick’s picture

Oh, and yes, manual is the only way with JS. :\ Although it's tricky to test this with just core, since afaik nothing in core actually uses jQuery UI...

mfer’s picture

Status: Needs work » Needs review
FileSize
447.73 KB

A patch that should work. Also note, The file misc/ui/images/ui-anim_basic_16x16_0.gif is not loger part of jquery ui and needs to be removed.

sun’s picture

Status: Needs review » Needs work

This patch cannot be tested, but it's OK to commit it.

+++ CHANGELOG.txt
@@ -191,7 +191,7 @@ Drupal 7.0, xxxx-xx-xx (development version)
-    * Added jQuery UI 1.8, which allows improvements to Drupal's user
+    * Added jQuery UI 1.8.5, which allows improvements to Drupal's user

We should not change CHANGELOG.txt.

+++ misc/ui/jquery.effects.transfer.min.js
@@ -1,15 +1,16 @@
+// $Id$

+++ misc/ui/jquery.ui.accordion.css
@@ -1,7 +1,16 @@
+/* $Id$ */

Actually, all JS files should use the former variant.

That said, I wouldn't mind to have no $Id$ CVS keywords in those files. We do not and will not change them on our end. So as long as they contain the proper version, there's no benefit of manually injecting a CVS Id tag. Would heavily simplify updates like this.

+++ misc/ui/jquery.ui.accordion.css
@@ -1,7 +1,16 @@
+ * jQuery UI Accordion @VERSION

Still contains plenty of @VERSION placeholders? I guess this means that this has to be fixed upstream.

Powered by Dreditor.

mfer’s picture

@sun Ideally we would just use the files from jQuery UI directly rather than modifying them.

jrabeemer’s picture

Sub

bojanz’s picture

Why not skip the $Id$ tags?
They will disappear anyway as soon as the Git migration happens (a script will go through all source and strip it), and that time is approaching fast.

Jeff Burnz’s picture

I think we have to have revision control on this file because there are modifications to it, as opposed to the core ui theme css which is a strait copy (correct me if I am wrong, not totally up with this issue).

webchick’s picture

This didn't make it into beta1, but it'd be nice to make into beta2. Any takers?

marvil07’s picture

Still contains plenty of @VERSION placeholders? I guess this means that this has to be fixed upstream.

As mentioned on the release blog post, some versions include @VERSION string. I suppose the problem was from where it was downloaded. So I think there is no a problem upstream(where I can see the right version).

@webchick: what is the official position about $Id$ ? (I suppose it is to include them, but I also think it is unnecessary since we do not touch them)

jrabeemer’s picture

Looks like this may not make it in before release.

amateescu’s picture

This patch addresses sun's comments in #20.

amateescu’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

Now is the perfect time to do this. Sufficient time to find any obscure bugs until the next beta/RC.

mfer’s picture

Status: Reviewed & tested by the community » Needs work
+++ misc/ui/jquery.effects.blind.min.js
@@ -1,15 +1,16 @@
+/* $Id$ */

According to the JavaScript Coding Standard the header blocks on JavaScript files should be in the form // $Id$

+++ misc/ui/jquery.ui.autocomplete.css
@@ -1,20 +1,34 @@
+/*
+ * jQuery UI Menu @VERSION
+ *

@VERSION should be replaced by 1.8.5. This is all over the code.

Powered by Dreditor.

amateescu’s picture

Status: Needs work » Needs review
FileSize
446.43 KB

I replaced all the js headers, and the @VERSION string was only in autocomplete.css because i manually replaced all the others in the first patch :) I guess this one slipped because it wasn't at the beginning of the file...

Status: Needs review » Needs work

The last submitted patch, jquery-ui-1.8.5_916968_32.patch, failed testing.

mfer’s picture

The failure should not be related to this patch. Did a failing test get in?

darvit’s picture

Updated point release for jQuery UI :

jQuery UI 1.8.6 has been released today.
http://code.google.com/p/jquery-ui/downloads/detail?name=jquery-ui-1.8.6...

lilou’s picture

amateescu’s picture

Title: Update to jQuery UI 1.8.5 » Update to jQuery UI 1.8.6
Status: Needs work » Needs review
FileSize
447.75 KB

New patch for jQuery UI 1.8.6.

marvil07’s picture

looks good, just to point the comment of mfer at #19: #916968-19: Update to jQuery UI 1.8.6, so we do not forget about that remove

webchick’s picture

I was about to commit this, but where on jQuery's site do you see 1.8.6?

http://jqueryui.com/download shows 1.8.5 as "stable" to me?

webchick’s picture

Status: Needs review » Fixed

Hm. I guess they have a GitHub tag and Google's CDN version is pointing at 1.8.6. I asked in #jquery and they told me 1.8.6 is indeed the latest, just awaiting a release announcement. I know a little something about that situation. ;)

Committed to HEAD.

xmacinfo’s picture

Priority: Major » Normal
Status: Fixed » Needs review
FileSize
766 bytes

Changelog patch specifying the new version of jQuery UI ready for review.

xmacinfo’s picture

By the way, #944308: Update to jQuery 1.4.4 had a changelog modification for jQuery. This is why I created a modification to the changelog for jQuery UI as well.

webchick’s picture

Status: Needs review » Fixed

Committed to HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

darvit’s picture

Title: Update to jQuery UI 1.8.6 » Update to jQuery UI 1.8.7
Status: Closed (fixed) » Active
geekgirlweb’s picture

+1 subscribing

neclimdul’s picture

Title: Update to jQuery UI 1.8.7 » Update to jQuery UI 1.8.6
Status: Active » Closed (fixed)

This is really completed. We're not going to drag this issue out with every point release of jQuery UI.
#1009862: Update to jQuery UI 1.8.7