MinnPost / object-sync-for-salesforce

WordPress plugin that maps and syncs data between Salesforce objects and WordPress objects.
https://wordpress.org/plugins/object-sync-for-salesforce/
GNU General Public License v2.0
95 stars 52 forks source link

When Using the `object_sync_for_salesforce_select_library` Hook to Disable the Select2 LIbrary Adding a New Field to a Mapping is Broken #406

Closed timnolte closed 2 years ago

timnolte commented 3 years ago

Describe the bug When using the hook object_sync_for_salesforce_select_library to disabled the use of the Select2 library, due to plugin conflicts related to this plugin not isolating it's use of this library, the result is that you can't edit an existing Field Map and add a new field. Clicking the "Add another field mapping" button yields no new line. Disabling the hook that changes the Select2 library restores this functionality.

To Reproduce Steps to reproduce the behavior:

  1. Use the hook object_sync_for_salesforce_select_library and return an empty string to return the Select functionality back to default browser Select behavior.
  2. Navigate to the Salesforce->Field maps.
  3. Edit an existing Fieldmap.
  4. Click on the "Add another field mapping" button. No new row is added.

Expected behavior When using the hook to disable the use of the Select2 library adding new field should still working using standard Select boxes.

Environment (please complete the following information):

Additional context ACF also uses Select2, but they have integrated it such that it is isolated from any other plugins that also use the library, so there is never any conflicts. This issue warrants another issue really. This plugin should be using the Select2 library such that i can be loaded side-by-side with other plugins using different versions of the library. ACF should be the model of how it should be implemented. I've opened up an issue for this item as well: #405

jonathanstegall commented 3 years ago

There's clearly a bug with select2 being disabled, and that is fixable with a quick JavaScript. I'm confused about the issue with The Events Calendar though. I run that as well and have never seen a conflict with it.

timnolte commented 3 years ago

So I believe we ran into this issue because we are actually loading some of The Events Calendar frontend forms into the backend on some custom admin screens. I'll have to double check with the other developer that was working on that functionality to confirm.

jonathanstegall commented 3 years ago

Yeah, more info about that would be good. It's also interesting because by default, this plugin uses Select Woo, not Select2, although those libraries both use the same JavaScript object. It doesn't seem to me that the way ACF loads would resolve this, but maybe it does. I'll see if I can integrate some of their method into the pull request, but I can't be sure that it works without having seen the issue.

timnolte commented 3 years ago

@jonathanstegall yeah, so I believe that The Events Calendar is also loading the Select Woo version of Select2.

jonathanstegall commented 3 years ago

@timnolte So the way ACF loads its version of Select2, it wouldn't detect the script from The Events Calendar. It's looking for scripts that are named select2, and TEC calls its script tribe-select2.

I think there are a couple of choices here, at least from my perspective:

  1. I'll definitely add the script change that's already in #407 so that it doesn't break the add new row with the default browser select. I'll merge this as soon as it's clear what to do aside from that (see below)

From here it's not as clear to me, but some further options:

  1. We could put a prefix on our version of selectwoo or select2, whichever is included. So it would be object-sync-for-salesforce-selectwoo. I don't think this would solve your problem, but it's worth a try.
  2. OR, I can rename our version to match how WooCommerce is loading it (they use selectWoo instead of selectwoojs for the script enqueue). I doubt this would fix the Events Calendar issue either, but I'm fine with doing it instead.

[Edit:] I looked into the ACF check, which works like this: if( isset($wp_scripts->registered['select2']) ) { but unless we have evidence that this would help, I'm not going to add this.

I'm open to other minor additions like this if you have them. But I'm inclined to think there's enough custom work going on with your addition of The Events Calendar front end scripts on the back end that unless one of the above things works for you, you should submit a pull request if you determine what works, and I can review it and see if it's something to add.

timnolte commented 3 years ago

@jonathanstegall so I did confirm with our other dev that the issue was with Add/Create New Venue option, and this was when the plugin was being loaded into the backend. I'm inclined to say that using the prefix approach might be what would work. If I have some time to test out one of those options I can see which solution works and submit a PR. I think we're good if we just have your initial code in place so that we can still add additional fieldmap rows when the Select library is disabled. Thanks!

jonathanstegall commented 3 years ago

@timnolte Ok then, I've merged that JavaScript fix, and I'll release it as a quick update shortly. I'll leave this issue open for now, and if you can get a PR for either option I'd be fine with adding it later as well. I would lean toward matching what WooCommerce is doing rather than a prefix, but if the prefix works and matching WooCommerce does not, that would be fine.

jonathanstegall commented 3 years ago

I've released the JavaScript fix as version 1.10.1.

jonathanstegall commented 2 years ago

Going to close this, with the understanding that it could be reopened if further issues arise.