Menu fabrik list Prefilter eval not being processed

B

Bauer

Well-Known Member
After updating from github yesterday the fabrik list menu items are not correctly processing any Prefilter options that use 'Eval'.

Instead it just returns the code itself.

Here's a cut and paste of the WHERE condition from the 'list GetData:' section in fabrikdebug...

WHERE ( master_report.user_id = 318 AND master_report.member_facility_id = 'return JFactory::getApplication()->getUserStateFromRequest(\'mfid\',\'mfid\',0);' AND master_report.facility_type_id = 'return JFactory::getApplication()->getUserStateFromRequest(\'ftid\',\'ftid\',0);' ) ORDER BY `master_report`.`uid_jc` ASC

'return JFactory::getApplication()->getUserStateFromRequest(\'mfid\',\'mfid\',0);'
'return JFactory::getApplication()->getUserStateFromRequest(\'ftid\',\'ftid\',0);'
is the code used for 'Value' in the last 2 prefilter items (set as Type: 'Eval') in the Menu configuration. (see attachment)

This had been working fine until now.:(
 

Attachments

  • menuPrefilters.PNG
    menuPrefilters.PNG
    44.5 KB · Views: 161
Predictably enough, works for me.

We have changed how menu prefilters work recently, by adding an option to the list's pre-filters for "Overridden by menu/module", which allows you to specify whether menu filters replace, or add to, list pre-filters.

What is that set to on your list, and do you have any other filters on the same element(s)?

-- hugh
 
cheesegrits said:
Predictably enough, works for me.

We have changed how menu prefilters work recently, by adding an option to the list's pre-filters for "Overridden by menu/module", which allows you to specify whether menu filters replace, or add to, list pre-filters.

What is that set to on your list, and do you have any other filters on the same element(s)?

-- hugh
Click to expand...
It's hard to keep up.
But why was this even needed/added? What was wrong with the way it was?
The menu pre-filters already specifically stated that "These filters will overwrite any list filters" - and IMO, that is the way it should work.

It seems like putting that option in the List configuration has it backwards. If you wanted an option to add the menu pre-filters to any existing list pre-filters (rather than overwrite), shouldn't you put that option in the Menu parameters? And wouldn't that have been better in regards to any concerns for 'backward compatibility' issues?

The same it true for all the CSV options. There really needs to be a way of overwriting the List CSV parameters in the menu parameters - because oftentimes the Menu has a different pre-filter and/or selected elements.

HERE'S WHAT I HAVE FOUND - and the main reason why I think it needs to be changed/fixed...

If (for CSV) I select 'Front End Options' in the list parameters, then the order of the 'Included fields' in the Menu parameters, including any fields from a joined table, is honored (it works as expected). But if I set the Front End Options to 'No' in the List parameters then the order of the included fields will always put the fields from the joined table at the end - in the last column(s). This is unacceptable and makes the front-end CSV export option useless to me.

I have been playing with this CSV feature for weeks now and realize all of its shortcomings. Wouldn't it be best that - in the list.js that handles the CSV export - there is only one section of code that always works the same way - and not have to maintain 2 different scenarios in that (ugh) mootoools code (based on the 'Front end options' setting in the list) - that produces 2 different 'fields' lists (one ordered correctly if joined tables are involved, and the other not) ??? I.e. the _csvAutoStart: function () vs. the _csvExportForm: function ()

The only way I could get it to work as I needed was to always use the _csvExportForm: function () - (Allow front end options set to 'Yes') - because that is the only way the order of the fields/columns is correct - per the images I posted here last week, ( http://fabrikar.com/forums/index.php?threads/a-few-quick-questions.43331/#post-221742 ) - but got no response.

But there are over a dozen files that are affected by my tweaks, and I don't see me having to figure out what Fabrik code needs changing, and 're-tweaking', every time I update from github. So, changing it to make it work the way I need it to work - and the way I think it should work - would basically mean putting a 'freeze' on any Fabrik updates for my project. (As has been the case while I'm working on this - and is why I never caught this latest issue until weeks after it was introduced). And that would basically mean the end of any 'beta testing' that I do - or the ability to use any new features you introduce.

To fix these shortcomings there should be a CSV tab included in the Menu parameters of the fabrik list Menu plugin. which would override the list settings. THAT is where the CSV parameters should be set - on a per-menu (page) level - not on the list level.

(And as I mentioned in other posts, I'd really like this to just be called the 'Import/Export' plugin - but I suppose the verbiage/labeling can be taken care of with language file overrides.)

Here is a minimal list of the parameters I'd like to see added to the CSV plugin (that I have added in order to make it work for me). With these parameters you would be able to...
  1. option->exportFileName - PHP code that would return a menu-specific or user-specific file name for the 'exported' file.
  2. option->exportFileExtension - A comma delimited list of possible file extensions - used to create radio buttons (in list.js - _csvExportForm: function) for the user to select the type of file they want (IF there is more than one option - defaults to just 'csv' if blank)
  3. option->saveFilePath - PHP code that would return the path to optionally store the file. e.g. in a 'user files area' (per another 3rd party extension being used for that purpose - like Phoca or Jsmallfib)
  4. option->exportPHP - PHP code that would be inserted (via php include) that would (using values from options 1,2,3) 'transform' the generated CSV into another file format (as set in 2.option, or some other 'user-preference' rules determined elsewhere). This code would be included after the CSV is generated, but before it is 'downloaded'.
This way the 'end result' of any so-called 'CSV Export' could be whatever file type is generated in item #4. In my case, it allowed me to transform the csv export file into a TRUE Excel worksheet, complete with cell formatting, locked cells, frozen header, etc. - via the phpExcel library.

IMO this would be a welcome feature addition to Fabrik - and it wouldn't take much work to implement.
 
Last edited:
But why was this even needed/added? What was wrong with the way it was? The menu pre-filters already specifically stated that "These filters will overwrite any list filters" - and IMO, that is the way it should work.
Click to expand...

Well, believe it or not, there are other people who have different requirements to you. I know, shocking, but one of the lessons I've learned in the last 10 years working on Fabrik is that everyone has firm, logical, reasonable opinions which differ wildly.

For instance, a lot of people (myself included) want a base set of immutable pre-filters which apply to every use case, and then apply additional filters to different menus. And because menu filters can be bypassed by simply removing or changing the itemid, this is the only way to do that, without copying the list for every menu item ... which kind of defeats the object of having configurable menu pre-filters.

It seems like putting that option in the List configuration has it backwards. If you wanted an option to add the menu pre-filters to any existing list pre-filters (rather than overwrite), shouldn't you put that option in the Menu parameters?
Click to expand...

Because anything which is menu setting dependent can be bypassed or messed with by removing or changing the itemid on the URL. We needed a way of having fixed and immutable filters which could not be bypassed in this way, that was copacetic with bypassable menu settings.

Once this change has settled in, we may look at extending this feature so the List option is "Yes, No, Per-Menu", and adding an "Override List Pre-Filters" to the menu settings, which would be consulted if "Per-Menu" set on the list.

But for now we were addressing a security issue which has been lurking behind the scenes for a long time.

And wouldn't that have been better in regards to any concerns for 'backward compatibility' issues?
Click to expand...

AFAIK, the default is 'overwrite' which makes it backward compatible, as you have to take an action to set that to No to change the behavior. I tested it when the changes were first committed. If that is not the case, and my tests were bogus, then we screwed up and I'll change it. But I just tested again, and it looks like all my existing lists have Overwrite set to Yes. And looking at the code, the XML defaults to "Yes" ...

Code: 
            <field name="menu_module_prefilters_override"
                  type="radio"
                  class="btn-group"
                  default="1"
                  description="COM_FABRIK_FIELD_LIST_MENU_MODULE_PREFILTERS_OVERRIDE_LIST_DESC"
                  label="COM_FABRIK_FIELD_LIST_MENU_MODULE_PREFILTERS_OVERRIDE_LIST_LABEL">
                <option value="0">JNO</option>
                <option value="1">JYES</option>
            </field>

... and where we fetch the param it defaults to true ...

Code: 
$overrideListPrefilters = $params->get('menu_module_prefilters_override', true);

I haven't responded to your CSV stuff yet because I'm busy, and it's a complex subject, and a long msg with a lot of question and proposed changes, that I need to devote considerable time to responding to. As soon as I get an hour or so to devote to it, I'll respond. It took me 10 minutes just to address your concerns about the pre-filter changes.

-- hugh
 
Last edited:
And here I thought sarcasm was just my thing. ;)

I get what you're saying - but my point is their seems to be a need for a hierarchy - between the list and menu params (not just here, but in numerous places, like for some plugin params) - and this only complicates that. And IMO the menu params should always take precedence.

"a overrides b" is simple enough. (both for the end-user to understand and for coding)
But now you have "a overrides b unless c is..."

I also agree that the list configuration is the logical place where the common/global list pre-filters should be. But if I was in your shoes I would have just added a radiobutton option in the menu/module params for the pre-filters with the options to either 'Add to' or 'Overwrite' the list pre-filters. Otherwise, it creates a situation in your code where you have to go back and forth between the 3 possible scenarios trying to figure out which one to use.

What if there are some instances where you would want the list prefilters to be "Overriden by menu/modules" - and some places where you don't? My question is whether the list parameters is where that option should be set - i.e. having it in the 'parent' list parameters prevents it from being implemented on an individual basis in each menu item, no? Or am I missing something here?

It seems like you (or much of the Fabrik code pertaining to list parameters) assume that each list will only have one front-end menu associated with it. But this particular list I am working with is being used in 8 different menu items - each with it's own unique prefilter and selected elements settings.

It's one of those chicken and egg things, but there needs to be consistency throughout the fabrik code - and IMO the final arbitrator should always be the menu/module parameter settings. What I am proposing is to move the CSV parameters into the menu/module parameters - because that is where it is most likely that there is going to be a need for unique prefilters, elements selected, and ALL of those CSV options that only show in the List parameters yet cannot be changed in the menu parameters. So why bother having it all in the List parameters? - unless you want to always use that only as a default if nothing is set in the menu/module parameters (which makes sense too, I suppose - and would eliminate any worries about 'backward compatibility')

As I mentioned - I have a similar issue with the 'Overwrite' option of the CSV Import settings. In some lists (menu pages), I don't want the user to be able to select that option, I want it to always be Yes - and yet (if the Frontend option isn't displayed, so the user can't change it) the default for the Overwrite will be 'No'.:(

So what I'd like to see for all of those Yes/No radio button settings in the 'csv_auto' section in the list.xml file is a 3rd option - 'User'. If 'User' is selected then the user gets to make the Yes/No selection in that mootools csv option popup on the front end - otherwise whichever of the other Yes/No options that was selected in the params would be used. Similarly, that is how my dilemma with the 'Overwrite' option for the CSV Import could be handled/solved too. ( I already have the code written to show/hide each of those options in the list.js file csv popup, as appropriate, using this method.)

This CSV import/export code is the biggest offender when it comes to having the ability to configure params based on the 'menu' list - vs. the 'list' list. But I have come to realize that there is a lot of code for 'import' and 'export' that shares functions - and a lot of the difficulty in attaining a more desirable and user-friendly functionality probably has a lot to do with some hysterical (my word for historical) code. And I understand how that happens over time, but it still doesn't mean I'd not like to be able to help to fix that.
 
Last edited:
I think you missed this bit:

Once this change has settled in, we may look at extending this feature so the List option is "Yes, No, Per-Menu", and adding an "Override List Pre-Filters" to the menu settings, which would be consulted if "Per-Menu" set on the list.
Click to expand...

Our immediate concern was more to do with security than the niceties of per-menu configurations.

Once we are happy that this code is solid and hasn't broken backward compat, we will look at making it more flexible.

-- hugh
 
cheesegrits said:
I think you missed this bit:

Our immediate concern was more to do with security than the niceties of per-menu configurations.

Once we are happy that this code is solid and hasn't broken backward compat, we will look at making it more flexible.

-- hugh
Click to expand...
Yeah I did miss that part. :oops:
This can't happen soon enough.

It's really not a matter of "niceties" - it's a matter of logic (what only makes sense). You already have the options to tweak the list params (filters, elements, etc) in the menus - so limiting the scope of those CSV params is very disappointing. IMO it should be that the admin can configure the CSV options as needed for each Menu/page - as well as set the "Front end options" to include everything that can be configured in the list params. You would expect that either you can change/override all, or you can't change/override any - otherwise you end up with some very frustrated customers left scratching their head as to how to make it work, like me.
 
I was specifically referring to a security issue taking precedence over anything else.

As for per-menu CSV params, that's a lot of work, which I don't recall anyone else ever asking for in the last decade, although I'm getting kind of senile so I may be wrong. We only have a certain amount of coding time for new features, so we have to spend it either implementing features we get asked for a lot, or that we get funded to do. A lot of people were asking to be able to do per-menu element and filters, so they happened.

-- hugh
 
I suppose I should just forget that the CSV Import/Export option even exists - which is what I did 3 years ago when I first realized it wouldn't do what I needed. It would be nice if it did, but I'm not interested in arguing about the benefits of my suggested improvements or how much work it would be.

The only reason I decided to reopen this can of worms is because I think other Fabrik users would welcome the changes - and I wanted to give something back to the community and perhaps add to the attractiveness and usefulness of Fabrik. You already know that I am willing to do the work - as I have to write the code to make it work this way in my project, one way or the other. I'm just looking for the approval/acceptance of my code from the Fabrik team (or at least some feedback on the feasibility of my proposed changes - on how they might affect other things that I don't even realize) - and mostly, so that I don't have to worry about my changes getting overwritten every time I update from github.

Also, I have found it impossible to create and maintain a working branch of the github code - at least found it to be more work than I have time or patience. So I'm stuck between a rock and a hard place here and feel as silly as a dog chasing his tail the past few weeks. I'm pretty much convinced now that my best bet would be to just go back to using the list PHP plugins to handle import and export of the data myself.

If you're really interested in anything other than support income - like what is really wrong with the Fabrik CSV Import/Export code and some of its shortcomings - I'll be happy to provide a list of the quirks and bugs that I know of - in the hopes that maybe one day someone will take the time to make it right.
 
You must log in or register to reply here.

Useful links

  • Create a Fabrik Account (Downloads)
  • Create a Forum Account
  • Fabrik Wiki
  • Fabrik 4 Changelog /Install /Upgrade
    • We are in need of some funding.
    More details.

    Thank you.

    Members online

    Total: 99 (members: 3, guests: 96)
    Back
    Top