[improvement] find_many returns an associative array

[Surt]

Pull based on #132

Added find_many return associative keys.
Micro optimizations for Idiorm and Paris.
set and set_expr returns the instance, to admit a fluent query

[lalop]

+1

[Surt]

On Paris, we can overload "_instances_with_key" instead "find_many". Using one loop less to fetch the query results.

   protected function _instances_with_key($rows){
            $size = count($rows);
            $instances = array();
            for ($i = 0; $i < $size; $i++) {
                $row = $this->_create_instance_from_row($rows[$i]);
                $row = $this->_create_model_instance($row);
                $key = (isset($row->{$this->_instance_id_column})) ? $row->{$this->_instance_id_column} : $i;
                $instances[$key] = $row;
            }

            return $instances;
        }

[treffynnon]

So my only real issue with this is that it is a backward compatibility break. I think at the minimum that this would need to be a configurable option. It would ship on, but you could disable it where required for compatibility with code bases that rely on sequential indexes in this array.

[lrlopez]

What about including this feature on a new method? Something along the lines of find_many_with_keys(). That way it wouldn't break backward compatibility...

[treffynnon]

Merged in commit 022d328 for Idiorm
and in commit j4mie/paris@9ac0ae7

[nachouve]

Hummm... maybe it is no so simple. I think $alias parameter should be a list of aliases, shouln't it?

[treffynnon]

Yes, it looks like you have a point there. if the select is an array of columns then it should be in the form:

$columns = array(
    'alias' => 'column_name',
);

[Surt]

Yep, on that particular commit I added the select to accepts comma separated values, but in that case you can't provide aliases, or it will fail. So you can use it as always, even using $alias or you can call it like:
->select('id, name,title, content') not using aliases.
There is no check if you are using the function on the right way so if you send a comma separated value and an alias you will have an unexpected behaviour.

[Surt]

The unique use of that commit was to reduce the notation to take multiple fields. However select_many does it already. (using more , and ` but in the right way.

[micmath]

I stumbled upon this change after I updated from 1.3 to 1.4 and my application started blowing up in mysterious ways. Am I to understand that this is a known backwards incompatible change to an existing API, and was released under a minor point release? Do you follow the semantic versioning pattern at all? http://semver.org/ Would you please?

[Surt]

@micmath don't know, but if your problem is related to the associative keys, it produces problems on

• tables with no primary id
• has_many_through

My original commit for this was changed in the official code in some ways and the results are not the ones you can expect.
There is a fix here: #162
It adds some new behaviours:

• If no primary key is defined numeric keys will be used on find_many
• results from has_many_through returns not associative keys
• new methods non_associative(), associative(), reset_associative() to override associative behaviour
• new config key to set associative keys by default or not (your code must be 100% compatible with 1.4 if you set this to false)

[micmath]

Thanks for the technical details, @Surt. I eventually managed to fix my specific issue, but it involved a panicked late-night coding session, knowing my client expected to go live today. This might be why I'm a tiny bit cranky this morning, but my larger point is that I shouldn't have to worry about backwards incompatible changes to existing APIs on a minor point release. (see the suggested link to semantic versioning)

[treffynnon]

@micmath Your petulance does nothing to resolve this situation or really make me all that sympathetic to be honest. The knock on effects were not anticipated when it was merged and I have been far too busy with client work to check and merge the subsequent patches. You can review them here #156

In the case of tables having no primary key: it is clearly documented that Idiorm assumes you have a primary key. if you implement differently then you're outside what is tested. If you implement something incorrectly then you can expect it to later be broken when something that assumes you have implemented it correctly is added to the code base. If you do this then it is important to check the changelog of any open source project to ensure that changes do not effect your non-conforming code. It has always been this way.

[micmath]

Thanks for your reply, @treffynnon. I honestly didn't realise I was being "petulant." I tried to say please and thank you, and did make the caveat that I am not my usual happy-bunny self this morning. So I apologise if I sounded pissy, I didn't intend it that way. But, again, I wasn't asking about primary keys, or any detailed technical explanation for that matter. (for the record my table does have primary keys) I actually believe the problem was to do with some quirk of how Twig was iterating over the result set, and changing it from an array in 1.3 to an associative array in 1.4, in my specific case caused Twig to sometimes panic for a reason I haven't yet diagnosed. (I eventually quelled the issue by using ->find_array() instead) But my point is more about the comments here–one by yourself, I believe–concerned that the "issue with this is that it is a backward compatibility break." It appears to me, from this thread, that a known backwards incompatible change to an existing API was made on a minor point release, and I am still wondering if that is the case and if that was the intention. And if it was, would you please consider using semantic versioning?

[treffynnon]

that a known backwards incompatible change to an existing API was made on a minor point release, and I am still wondering if that is the case and if that was the intention.

Backwards incompatible changes are avoided in all merges. The knock on effects were not anticipated when this particular change was merged. Mea culpa - I was rushing.

The release was also open for testing and I did put out a call for help testing before releasing, but no issues were raised.

consider using semantic versioning

The project has been since https://github.com/j4mie/idiorm/releases/tag/semver

[treffynnon]

@micmath I would be interested to hear what the trouble with Twig is because I tested this release against a project of my own in addition to the unit tests. That project used Twig to iterate over find_many() results both returned as an array and as a result set.

[micmath]

I was planning to go through the issue with a fine-toothed comb when I have more time and caffeine. But if it helps in the short-run at all, I only saw the problem when there were NULL values in the result set returned from find_many(). I’ll include the Twig code in question:

{% for row in rows %}
<tr>
  {% for col in cols %}
<td>{{ attribute(row, col) }}</td>
  {% endfor %}
</tr>
{% endfor %}

where rows is the result set returned from find_many(), and cols is an array of stringy column names I want to render. Using the exact same data table the above renders as expected using Paris 1.3, but when using Paris 1.4 Twig appears to fall over if any of the column row values are NULL.

[Surt]

Hi @micmath did you tried this?

strict_variables: If set to false, Twig will silently ignore invalid variables (variables and or attributes/methods that do not exist) and replace them with a null value. When set to true, Twig throws an exception instead (default to false).

http://twig.sensiolabs.org/doc/api.html#environment-options

I think that you can use this notation on the last foreach too

{{ row[col] }}

[lrlopez]

I also experienced some problems with Twig templates that were solved by using find_array() instead of find_many()... Should I came again with this, I will try to debug the cause.

[treffynnon]

@micmath and @lrlopez could this be the same thing as @amerker is reporting on Paris? See j4mie/paris#75 (comment)

It would be really helpful if anyone could include a regression test or some code I can use to replicate this error to take this issue forward. Unfortunately I am really busy with pre-Christmas client deadlines, a course and general family life at the moment so it is making it exceedingly difficult to work on Idiorm and Paris so any additional help is very welcome and gratefully received.

[treffynnon]

@micmath , @lrlopez and @amerker please could you also try the proposed patch and let me know how it goes and whether it is good to merge or not.

Please see: #156 (comment) and the pull request at #162

I would like to fix this regression ASAP as it is clearly affecting a significant proportion of Idiorm users. If the patch is not good enough then I am going to have consider rolling out a 1.4.1 release without the presence of the primary key as associative array key feature included.

Please do let me know your thoughts as I would like to resolve this quickly and in a way you guys can use it/agree with.

[lrlopez]

I've tested my current project with #162 and it seems to work well, although I haven't used any of the new methods but the default behavior. The only thing I miss in the PR are tests.

About the problems with Twig templates, I suppose it is the same issue described by @amerker because no error was logged anywhere and a infinite recursion could explain that...

[amerker]

Sadly, the problem persists for me with Surt's current regression.find_many_id_keys branch.
Here's a barebones system that should demonstrate the problem:

DB (MySQL):

CREATE TABLE IF NOT EXISTS `twig_test` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `requiredField` varchar(128) NOT NULL,
  `nullableField` varchar(128),
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8;
INSERT INTO `twig_test` (`requiredField`, `nullableField`) VALUES ('first set', 'optional first data');
INSERT INTO `twig_test` (`requiredField`) VALUES ('second set');
INSERT INTO `twig_test` (`requiredField`, `nullableField`) VALUES ('third set', 'optional third data');

Model and Slim Route:

class TwigTest extends Model {}
$app->get(
    '/twigtest',
    function() use ($app) {
        $testdata = Model::factory('TwigTest')->find_many();
        $app->render('twigtest.html.twig', ['testdata' => $testdata]);
    }
);

Twig:

{% for row in testdata %}
    {{ row.requiredField }} // {{ row.nullableField }}<br/>
{% endfor %}

This always throws an infinite recursion, which my Xdebug reacts to with:

Fatal error:  Maximum function nesting level of '100' reached, aborting! in /[...]/paris.php on line 522

Twig does not render the third output line (= data set) after that.
As soon as I replace find_many with find_array in the Slim Route, it all works.

This is using j4mie/paris 1.4.1, slim/slim 2.3.5, slim/views 0.1.0, twig/twig 1.14.2.

[Surt]

Hi @amerker did you try setting the new variable in config: 'find_many_primary_id_as_key' => false, to false? That one makes idiorm behave as old days, without associative arrays.

Anyway I don't think that bug is related to this patch, since the assignement to rows (where the null is assigned) is done prior to the primary keys assignement.

[amerker]

Hi @Surt, you're right, my bug occurs no matter if the variable is true or false. I'm opening a new Paris issue, then.

[Surt]

@amerker by the way, if you are using Paris, and my regression branch for Idiorm, be sure to make this changes to Paris. It affects to the assignement but again, I don't think the nullableField and regression problems are related to this.

We must change in https://github.com/j4mie/paris/blob/master/paris.php#L128

   /**
         * Create instances of each row in the result and map
         * them to an associative array with the primary IDs as
         * the array keys.
         * @param array $rows
         * @return array
         */
        protected function _instances_with_id_as_key($rows) {
            $size = count($rows);
            $instances = array();
             for ($i = 0; $i < $size; $i++) {
                $row = $this->_create_instance_from_row($rows[$i]);
                $row = $this->_create_model_instance(row);
                $key = (isset($row->{$this->_instance_id_column}) && $this->_associative_results) ? $row->{$this->_instance_id_column} : $i;
                $instances[$key] = $row;
            }
            return $instances;
        }

And the last change, in Model, https://github.com/j4mie/paris/blob/master/paris.php#L388

       ->where("{$join_table_name}.{$key_to_base_table}", $this->$base_table_id_column)
       ->non_associative();

Until this patchs, Paris is broken on has_many_through, returning just 1 instance of multiple objects with the same id (the objects are pushed on the same key)

I can't make a fork of Paris since https://github.com/Surt/Granada and other forks are based on it and I'm so bad at git to make by my own :p

[amerker]

@Surt, I changed the code manually, but it didn't help. So yes, I'm guessing it's not related. :)
BTW, you're doing two $row = assignments one after the other, is this intended?

[Surt]

yep, maybe we can change it to be more concise.
The first assignement takes the idiorm result, the second creates the model instance.