-
-
Notifications
You must be signed in to change notification settings - Fork 814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRM-17352 - Use Backbone.noConflict #11504
Conversation
Should we close #9759 in response to this? |
Test failures make no sense to me. Retest this please. |
test this please |
@colemanw @jackrabbithanna @dsnopek I tried testing this out with drupal 8 and it appears that the new script is being loaded too early this is what the sources looked like in my testing of the manage contribution page
This was leading to
|
I don't know if this is related, but I've seen some issues with Javascript files being loaded too early and my theory was that it was because they were intended for the footer, but since D8's CiviCRM integration can't yet put scripts in the footer, it was putting them in the header and too high up. I never got around to verifying that, though - the specific broken Javascript was low priority. |
@seamuslee001 is that the complete header? I don't see the Backbone scripts in there at all :/ |
@colemanw no they were getting loaded but much further down. After the navigation menu js etc. I can post fuller tomorrow if needed. |
@seamuslee001 I've just pushed another change which might solve the problem. It's a little heavy-handed but is probably the safest way to avoid edge-case bugs related to script ordering. |
@colemanw Can you share what the |
Sorry I haven't had time to spin up a D8 sandbox to test this on. I've verified that it doesn't break anything on D7, and GH just enthusiastically gave it a thumbs up for fixing the bugs in Wordpress. I'm confident this should do the trick on D8 but will need someone to try it out. |
Ok, I'll see if I can help test - just reading the details of the issue now :-) |
I did some testing on Drupal 8! Since I'm new to this issue, it wasn't totally clear to me how to reproduce. This is what I tried (please let me know if I'm doing this wrong):
Expected: I would have the ability to add another profile Actual: the screen scrolled up to the top of the page and nothing changed. No Javascript errors in the console. Then I applied both the changes here and the changes on civicrm/civicrm-packages#198 and cleared all the caches Following the same steps, I get the same result, except now there are numerous Javascript errors:
Looking at the page source, the
Please let me know if I did anything wrong! I'd be happy to test again. :-) |
Hmm @dsnopek those scripts look correct to me. Can you also clear your browser cache? |
I tried clearing caches - that didn't help. The Javascript errors are basically saying that I tried running So, here's appears to be happening:
If I change this line:
to:
That gets rid of the errors from crm.backbone.js. However, I think it'd probably be better if crm.backbone.js depended on I'm still getting this Javascript error from crm.designerapp.js:
I'm really not sure why my hack doesn't work there too? However, probably all these Javascript files that refer to |
Gah! Nevermind. :-) I messed up applying the patches. I forgot that because I'm running with the vendor directory outside of the webroot, that I needed to copy any changes to assets into the webroot. We have a build script that does that, but since I was applying these patches by hand, I was forgetting that step. Undoing my hack and just working straight with the patches is now working in my testing! Sorry for all noise... And 👍 |
Ok so this has now been tested successfully on D7, D8 and Wordpress. I think we're in good shape to merge. I'll wait another day for any additional feedback & then merge if there are no objections. |
Oh wait I just saw your last comment @seamuslee001. Do you have any errors in your console? |
@colemanw there aren't any console errors. if anything it looks like a style problem because if you look right at the bottom of that screenshot you will see the profile stuff there but its just not in the modal area and not on the left like it usually is. I've attached an updated screenshot which shows the issue a bit more cleanly |
@colemanw i think i have solved the display problem see PR here colemanw#5 |
Jenkins re test this please |
Jenkins re test this please |
Test failures are unrelated, it was passing before and the only update was to CSS. |
@colemanw i think the test failure is coming from https://github.com/civicrm/civicrm-core/pull/11507/files where https://github.com/civicrm/civicrm-core/pull/11507/files#diff-7b0caad195353c8c5d49bbf5f053daf6R89 isn't being picked up should be indented an extra 2 spaces because it doesn't seem to understand the finally stuff |
I'm running 5.2.1 in Joomla and getting the following js errors: JQMIGRATE: Migrate is installed, version 1.4.1 |
@kngs perhaps try to get help on chat.civicrm.org - the error is maybe that the script files are not being loaded? |
This was due to a conflict with the Grants Application extension which has been upgraded and is compatible with 5.x so is now resolved. |
Overview
Fixes JS errors caused by the presence of other versions of Backbone (e.g. in Drupal 8 or certain WP extensions).
Before
Profiles cannot be edited on the "Manage Event" screen etc. in D8 or certain WP sites.
After
Profile editing works normally regardless of other js plugins present.
Notes
This requires civicrm/civicrm-packages#198 - use both PRs when testing.