Skip to content
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

Update JFormFieldSQL to use JDatabaseQuery #3476

Closed
wants to merge 11 commits into from

Conversation

fastslack
Copy link
Contributor

Use JDatabaseQuery

This PR update the JFormFieldSQL input field to use JDatabaseQuery with separated query elements.
Into your form xml file you can set the query using this way:

<field name="example_group" 
type="sql" 
label="COM_EXAMPLE_GROUP" 
sql_select="e.*" 
sql_from="#__example AS e" 
sql_group="name" 
sql_order="e.id ASC" 
key_field="id"
value_field="name" 
onchange="this.form.submit();" />

This example will recreate this query:

SELECT e.* FROM #__example AS e GROUP BY name ORDER e.id ASC

Filters

One posibility that offer use JDatabaseQuery instead the old way is to have a way to get "inline" filters. That mean that if you have multiple dropdowns as filter in your list view, its posible that you need to connect it togheter.

For example, we have 2 dropdowns that filter our list, one called "groups", another called "subgroups":

<field name="groups" 
type="sql" 
label="COM_EXAMPLE_GROUP" 
sql_select="e.*" 
sql_from="#__example_groups AS e" 
sql_group="name" 
sql_order="e.id ASC" 
key_field="id"
 value_field="name" 
onchange="this.form.submit();" />

And the second:

<field name="subgroups" 
type="sql" 
label="COM_EXAMPLE_GROUP" 
sql_select="e.*" 
sql_from="#__example_subgroups AS e" 
sql_group="name" 
sql_order="e.id ASC" 
sql_filter="groups"
key_field="id"
 value_field="name" 
onchange="this.form.submit();" />

Note that we added sql_filter="groups", that search for groups changes and set the condition if its founded. So we can have a 'inline' filter on our lists. This will recreate:

SELECT e.* FROM #__example_subgroups AS e WHERE `groups` = 99 GROUP BY name ORDER e.id ASC

@fastslack
Copy link
Contributor Author

@zero-24
Copy link
Contributor

zero-24 commented Aug 9, 2014

JC is http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_id=8549&tracker_item_id=33628

@fastslack have you a easy way how we can test this? Or a example how this can/is used?

Cood looks ok here (I will comment for CS issues). Thanks.

$query['from'] = (string) $this->element['sql_from'];
$query['join'] = $this->element['sql_join'] ? (string) $this->element['sql_join'] : '';
$query['group'] = $this->element['sql_group'] ? (string) $this->element['sql_group'] : '';
$query['order'] = (string) $this->element['sql_order'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use here blanks that all equal sign on the same high.

@fastslack
Copy link
Contributor Author

@zero-24 If you have a component list into the administration side that use filters (using JModel::getfilterForm) the file to edit is file located in /models/forms/filter_yourlist.xml . Look at the explanation of filters there are a good example of how to test it. You can add tags with filters, so your inputs will modify "inline" with the new filter status.

*
* @return $query The query object.
*
* @since 12.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fastslack i think this can be changed from 12.1 to 3.4

@zero-24
Copy link
Contributor

zero-24 commented Sep 11, 2014

@fastslack i have it just tested.

Please confirm that it was correct

First I apply your patch.

I'm using this xml definition

<field
 name="example_group" 
 type="sql" 
 label="COM_EXAMPLE_GROUP" 
 sql_select="c.*" 
 sql_from="#__content AS c" 
 sql_group="title" 
 sql_order="c.id ASC" 
 key_field="id"
 value_field="title" 
 onchange="this.form.submit();"
/>

on the edit.php i have add this lines:

<div class="control-group">
 <div class="control-label"><?php echo $this->form->getLabel('example_group'); ?></div>
 <div class="controls"><?php echo $this->form->getInput('example_group'); ?></div>
</div>

The result is the following:
screen shot 2014-09-11 at 11 38 16

The Debug Console show this sql query

SELECT c.*
FROM iv1sg_content AS c
GROUP BY titl

Is this correct?

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@fastslack
Copy link
Contributor Author

Yes, that is correct but also, we can use it to filter lists too. Check this example:

We have a simple list with 3 filters to apply:

List

Into the file com_components/models/forms/filter_ourlist.xml we have:

<field name="place" type="msql" default="1" label="COM_EXAMPLE_PLACE" 
  sql_select="t.id, t.name" sql_from="#__places AS t" 
  sql_where="t.published = 1" sql_order="t.id ASC"
  key_field="id" value_field="name" onchange="this.form.submit();" />

<field name="zone" type="msql" size="1" default="" label="COM_EXAMPLE_ZONE" header="COM_EXAMPLE_ZONE"
  sql_select="t.id, t.name" sql_from="#__zones AS t" sql_where="t.id != 0" sql_order="t.name ASC" sql_filter="place"
  key_field="id" value_field="name" description="PARAMFILE" onchange="this.form.submit();" 
/>

<field name="muscle_group" type="msql" size="1" default="" label="COM_EXAMPLE_MUSCULAR_GROUP" header="COM_EXAMPLE_MUSCULAR_GROUP"
  sql_select="t.id, t.name" sql_from="#__muscles_groups AS t" 
  sql_where="t.enabled = 1 AND t.id != 0" sql_order="t.name ASC" sql_filter="" 
  key_field="id" value_field="name" description="PARAMFILE" onchange="this.form.submit();" 
/>

See that the zone filter has some argument called sql_filter with the value place. So this means that zone will check inline for changes on "place" input, and apply to the same list the filter.

Filter 1

And if we change the "place" filter.

Filter 2

Same is we apply the filter and we decided to made a new article, if the form to create a new article has the input "zone" the filter will be applied.

Check the latest file here:

https://github.com/fastslack/matware-libraries/blob/master/libraries/matware/form/fields/msql.php

@zero-24
Copy link
Contributor

zero-24 commented Sep 12, 2014

Yes that looks cool here. Can you have a look into the CS things i comment? If it is ok i can send a quick PR against yours that can fix the CS related things. 😃

@fastslack
Copy link
Contributor Author

Looking for CS, give me a moment...

@zero-24
Copy link
Contributor

zero-24 commented Sep 12, 2014

👍

@fastslack
Copy link
Contributor Author

Done

@zero-24
Copy link
Contributor

zero-24 commented Sep 12, 2014

thanks @fastslack

Sorry but Travis find too more cs issue that i have not seen:

FILE: ...ome/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/sql.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 168 | ERROR | Expected 2 spaces after the longest variable name
 208 | ERROR | Expected "foreach (...)\n...{\n"; found "foreach(...)\n...{\n"
--------------------------------------------------------------------------------

@fastslack
Copy link
Contributor Author

Fixed travis errors

@zero-24
Copy link
Contributor

zero-24 commented Sep 12, 2014

Hehe Travis does not give up 😄

FILE: ...ome/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/sql.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 166 | ERROR | Extra newline(s) found before function comment short description
 169 | ERROR | Expected 2 spaces after the longest variable name
--------------------------------------------------------------------------------

@zero-24
Copy link
Contributor

zero-24 commented Sep 25, 2014

@test @fastslack my example code: #3476 (comment) still works ok here.

@fastslack
Copy link
Contributor Author

@b2z @zero-24 Thanks very much for testing. Is there is another step to merge it?

// Process the filters
if (!empty($filter))
{
$html_filters = JFactory::getApplication()->getUserStateFromRequest($this->context . '.filter', 'filter', array(), 'array');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested yet, but this looks a bit fishy to me:
You take an input from the request and pass it unescaped and unfiltered into the query on line 212? I doubt that's going to be a good idea.

Also I'd use $db->quote() and $db->quoteName() to correctly quote things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right about quoting, i'm fixing the PR right now. But if this code is fishy, current stable Joomla! sql.php field has a big security issue because it is not filtering the query at all and it allow to put the entire query directly. Remember that only users with +rw privileges can exploit it.

https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/sql.php#L161

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users with write permissions could do a lot more. So I think it's safe to threat the XMLs as a trusted source.
My concern here is only about the filters which you take directly from the request (using getUserStateFromRequest). I assume filtering for array doesn't do restrictive filtering on the actual values, but I may be wrong. I haven't looked at that thing for quite some time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUserStateFromRequest($this->context . '.filter', 'filter', array(), 'array') will indeed only force the input to an array. You could pass any value you want using &filter=foobar in the URL and it will end up in the query. So you indeed need to sanitise this value before using or we will get a security issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it Tom :) Working on sanitise and quote.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope to see it soon.

@fastslack
Copy link
Contributor Author

Hi guys, please test the latest updates. I added default values options so we can set:

sql_default_{FILTER_NAME}="1"

If the filter value is empty, the default value for the custom field name is added to query.

Also added security checks for sql query to prevent injections.

@brianteeman
Copy link
Contributor

As there have been changes to the Pull Request after the tests I am resetting the tests - we need two tests on the latest PR thanks

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3476.

@chmst
Copy link
Contributor

chmst commented Mar 14, 2015

Works only with correct syntax. Otherwise - Warnings or destroyed Template


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3476.

@fastslack
Copy link
Contributor Author

@chmst can you give me an example? because i think that the current sql field destroy the template if syntax is wrong too

@chmst
Copy link
Contributor

chmst commented Mar 20, 2015

You are right - the behaviour is the same if a query is not well formed.
I've repeated the test - and with your query it works.
But I could not add a where clause. So this feature is not very useful for me.
Maybe you can expand your code?

    <field
         name="test" 
         type="sql" 
         label="test" 
         query="select c.id, c.title from #__content as c where c.title like '%Begleitservice%'"
         key_field="id"
         value_field="title" 
         onchange="this.form.submit();"
        />

makes a dropdown with 2 entries as expected for my database.

    <field
         name="test" 
         type="sql" 
         label="test" 
         sql_select="c.id, c.title" 
         sql_from="#__content as c" 
         sql_where="c.title like '%Begleitservice%'"
         sql_group="c.title" 
         sql_order="c.id ASC" 
         key_field="id"
         value_field="title" 
         onchange="this.form.submit();"
        />

gives the whole list of entries in the table.

@fastslack
Copy link
Contributor Author

Of course, i ll check this. Thanks for testing!

@zero-24
Copy link
Contributor

zero-24 commented May 3, 2015

@fastslack did you get time to check the where as suggested by @chmst ?

@fastslack
Copy link
Contributor Author

@chmst please test now, sorry about the delay

@chmst
Copy link
Contributor

chmst commented May 8, 2015

Hi, I did it - and it was successful. I tested on a database with > 1000 articles and both variats, the old and the new one gave the same result:

Note: I did not test join ...


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3476.

@zero-24
Copy link
Contributor

zero-24 commented May 8, 2015

@fastslack i have just also retetsted with the codes above.

But i know it is hard to tell but Travis is again not happy.

1) JFormFieldSQLTest::testGetInput
Undefined property: JFormFieldSQL::$header
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/sql.php:79
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/sql.php:273
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/list.php:58
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/field.php:393
/home/travis/build/joomla/joomla-cms/libraries/joomla/form/fields/sql.php:82
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/fields/JFormFieldSQLTest.php:108
phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:152
phar:///home/travis/.phpenv/versions/5.3.29/bin/phpunit/phpunit/TextUI/Command.php:104

https://travis-ci.org/joomla/joomla-cms/jobs/61804403

It looks like he is not happy with the new $header option?

@fastslack
Copy link
Contributor Author

@zero-24 Seems that Travis is happy now :-D

@zero-24
Copy link
Contributor

zero-24 commented May 9, 2015

Thanks @fastslack Based on the tests here i move to RTC. 👍


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3476.

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label May 9, 2015
@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jun 11, 2015
@mbabker
Copy link
Contributor

mbabker commented Jul 11, 2015

Merged to 3.5-dev via 30a953e

@mbabker mbabker closed this Jul 11, 2015
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants