Refactoring / Version 7.x-3.x [#2165425]

Problem/Motivation

I tried to use the module in my scenario but I wasn't able to cover all the use cases.
While patching the code for me the question arose why the module relies on views but doesn't use the views ajax handling but instead does all on it's own.

Proposed resolution

Refactor the code to fully use the capabilities of the views ajax handling.
I've already re-factored vast parts of the code with and added pieces from code I wrote for another project.
Because this is quite a bit of work and a patch would be very big I decided to create a fork of the module in a sandbox: https://drupal.org/sandbox/daspeter/2165417
git clone --branch 7.x-3.x http://git.drupal.org/sandbox/daspeter/2165417.git

So what's the benefit of the re-factored code:

Smaller, and hopefully easier readable, code base
More functionality. E.g. with following patch and sandbox you can split a view into pieces and use panels to place the pieces wherever you'd like on the page and ajax still works:
#2035657: More flexible / defensive ajax handling.
Views Content Types

In my limited test-setup the stuff works as expected. However further testing is necessary!

Remaining tasks

Reviews / testers needed.

User interface changes

Option to toggle ajax mode for ajxa widgets is gone.
Removed the tooltip functionality

API changes

Whole new "API".

Comment	File	Size	Author
#16	2165425-ajax_facets-3x-16.patch	46.24 KB	hefox
#15	2165425-ajax_facets-3x-15.patch	0 bytes	hefox
#14	2165425-ajax_facets-3x-14.patch	45.41 KB	hefox
#13	2165425-ajax_facets-3x-13.patch	45.23 KB	hefox

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

eugene.ilyin CreditAttribution: eugene.ilyin commented 1 January 2014 at 16:09

What do you offer me? Investigate your module and if I like it, to refactor my module?

Comment #2

das-peter CreditAttribution: das-peter commented 1 January 2014 at 16:43

Well I offer you a reduced and hopefully more readable / easier maintainable code base in which I invested quite some time writing.
If you like the approach we could start a new branch / version for your module and pull the code from my sandbox into your module (that's why I called the branch in my sandbox already 7.x-3.x).

There's no need to do another re-factoring as I did it already. It's now up to you if you see a future for the approach.
I don't see a reason why I should create another module if there's already one.
If you find any showstopper for a merge just let me know and I see if I can solve it.

Comment #3

eugene.ilyin CreditAttribution: eugene.ilyin commented 1 January 2014 at 16:45

Okay, I'll look at your code in the near future, thank you for your work.

Comment #4

das-peter CreditAttribution: das-peter commented 1 January 2014 at 16:53

Sounds awesome :)
If there are any questions we can discuss those via IRC - I should be around quite often.

Comment #5

eugene.ilyin CreditAttribution: eugene.ilyin commented 1 January 2014 at 17:04

Great!

Comment #6

eugene.ilyin CreditAttribution: eugene.ilyin commented 6 January 2014 at 06:12

Just clarification, your patch from this issue #2035657: More flexible / defensive ajax handling. should be approved for your new functionality of ajax_facets?

Comment #7

das-peter CreditAttribution: das-peter commented 6 January 2014 at 08:48

Hmm, I'm not even sure about that :) The patch is surely necessary if you want to use the search page view with ctools views content panes to split the view into pieces.

Comment #8

eugene.ilyin CreditAttribution: eugene.ilyin commented 6 January 2014 at 08:56

I mean that when you refactored ajax_facets, it requires functionality from #2035657: More flexible / defensive ajax handling. for works correctly?

Comment #9

das-peter CreditAttribution: das-peter commented 6 January 2014 at 11:07

I don't think so - but to give you a 100% accurate answer I would have to setup a clean installation and test that.

Comment #10

eugene.ilyin CreditAttribution: eugene.ilyin commented 6 January 2014 at 11:09

Please do it. I'm waiting your answer.

Comment #11

das-peter CreditAttribution: das-peter commented 6 January 2014 at 18:46

I've tested this now and the patch is required because without it the ajax configuration (the instance stored in the Drupal.views.instances['views_dom_id:' + Drupal.settings.ajax_facets.view].refreshViewAjax) of views becomes unreliable.
Btw. I found another bug while testing and fixed it - so if you've already cloned the sandbox please make sure you pull again ;)

Comment #12

eugene.ilyin CreditAttribution: eugene.ilyin commented 7 January 2014 at 03:10

So, then I should wait when your patch from issue #2035657: More flexible / defensive ajax handling. will be approved.

Comment #13

hefox CreditAttribution: hefox commented 15 April 2014 at 22:07

File	Size
2165425-ajax_facets-3x-13.patch	45.23 KB

This is a patch of -dev to -dev for use in make files so don't need to reference a sandbox project.

working nicely with the views patch.

Comment #14

hefox CreditAttribution: hefox commented 15 April 2014 at 22:15

File	Size
2165425-ajax_facets-3x-14.patch	45.41 KB

Quick reroll cause search_api_solr => search_api (will file a separate issue for that but meh, might as well just have one patch)

Comment #15

hefox CreditAttribution: hefox commented 15 April 2014 at 22:19

File	Size
2165425-ajax_facets-3x-15.patch	0 bytes

Missed a file

Comment #16

hefox CreditAttribution: hefox commented 15 April 2014 at 22:21

File	Size
2165425-ajax_facets-3x-16.patch	46.24 KB

3 files were hidden/shown/deleted

File	Size
2165425-ajax_facets-3x-13.patch	45.23 KB
2165425-ajax_facets-3x-14.patch	45.41 KB
2165425-ajax_facets-3x-15.patch	0 bytes

Empty file? o.O

Comment #17

hefox CreditAttribution: hefox commented 15 April 2014 at 22:26

Note for those looking for links: the sandbox adds that :)

Comment #18

eugene.ilyin CreditAttribution: eugene.ilyin commented 2 July 2014 at 07:19

I started working with new version. 7.x-3.x. Unfortunately issue #2035657 still not solved and I cannot apply changes from your fork.

Now I'm using Drupal ajax instead of custom ajax. I'm not sure that use view ajax handling is best solution, because some users want to use this module with "current search". I'll check this possibility.

Thank you for your fork and interesting to this module.

Comment #19

ravi.chand06 CreditAttribution: ravi.chand06 as a volunteer commented 7 September 2015 at 05:26

Hi,

I get the below error:

An AJAX HTTP error occurred.HTTP Result Code: 200Debugging information follows.Path: /ajax/ajax_facets/refreshStatusText: OKResponseText:

( ! ) Fatal error: Call to a member function execute_display() on a non-object in \sites\all\modules\ajax_facets\ajax_facets.pages.inc on line 43
Call Stack
# Time Memory Function Location
1 0.0000 385736 {main}( ) ..\index.php:0
2 0.5114 41635440 menu_execute_active_handler( ) ..\index.php:21
3 0.5114 41656056 call_user_func_array:{C:\wamp\www\takamolholdings\includes\menu.inc:517} ( ) ..\menu.inc:517
4 0.5114 41656576 ajax_facets_refresh_facets_content( ) ..\menu.inc:517

I created a page view and enabled the AJAX from views. Previously it triggered page SUBMIT via GET method, now it triggers AJAX but then I get the above return response.
Any suggestions would be helpfull.

Thank you.

Comment #20

eugene.ilyin CreditAttribution: eugene.ilyin commented 22 September 2015 at 16:58

Which version of module do you use?

Comment #21

eugene.ilyin CreditAttribution: eugene.ilyin commented 23 September 2015 at 16:07

Status:

Needs review

» Postponed (maintainer needs more info)

Comment #22

eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commented 3 December 2015 at 12:57

I don't think that 7.x-3.x version will be refactored because I'll concentrate on developing of D8 version and supporting of D7 version without major changes.

Thank you all for your efforts. If you'll have some troubles, please open new issues.