Reviewing the Unfounded branch

[Jeff Moore-4]

Hi Guys,

This is my review of the proposed changes in the beta-unfounded branch  
regarding which result is returned when a query returns no results and  
what appears in a related property when there is no related object.

In summary, I think that the changes for collections are beneficial,  
but marginally so, while the changes to record are clearly a net  
negative.

I'd like to address the changes to collections and to records  
separately.


RECORD CHANGES IN UNFOUNDED

Let me go into detail on the Record changes first.  The unfounded  
branch changes the return result of fetchOne methods to return an  
Empty Record instead of returning NULL.

This requires that code of this form

$item = $model->fetchOne();
if (! $item) {
        ...
}

Must be changed to

$item = $model->fetchOne();
if ($item->isEmpty()) {
        ...
}

Additionally, in models any hasOne or BelongsTo relationship will also  
change.  Instead of the property containing NULL when there is no  
record, the property will contain an "Empty Record" to indicate that  
there is no record in the relationship.

The "Empty Record" is a special state where some operations are not  
allowed, such as delete and result in exceptions, while other  
operations such as fetching properties are allowed.

A special behavior is included where by if a property value is set on  
an empty record, it automatically becomes a new record as if it was  
retrieved with fetchNew().

The NULL behavior is not broken and doesn't need to be fixed.


A. Principle of Least Astonishment

I think this new syntax violates the principle of least astonishment.  
The convention of using NULL or FALSE to indicate not found is well  
established in the PHP community and in other programming languages.

Receiving a record from fetchOne() where no record should exists is  
"Astonishing," and not not in a good way.

The paradigm requires extra effort to learn and adds to the Solar  
learning curve.


B. Meager Benefit I

I'm told the benefit to changing how not found is processed is to make  
the interface between Records and Collections similar.

"The flip side of the Rule of Least Surprise is to avoid making things  
superficially similar but really a little bit different.  This is  
extremely treacherous because the seeming familiarity raises false  
expectations.  It's often better to make things distinctly different  
than to make them almost the same." -- Henry Spencer

We have two concepts here, "no records" (empty collection) and "no  
record".  Attempting to represent "no record" with a special record  
that has the same interface as an empty collection ("no records") is  
only going to lead to confusion.  If you are confused by that  
sentence, you are proving my point.

Which interface is more clear?

$item = $model->fetchOne();
if ($item->isNotFound()) {
        ...
}

Or

$item = $model->fetchOne();
if ($item->isEmpty()) {
        ...
}

I think calling isNotFound() is more clear than calling isEmpty() when  
trying to determine fetchOne() found a record or not.

However, Implementing a special "Not Found" State in the Record object  
does not achieve unification with the empty logic in the Collection  
class.

Another way to unify the interfaces would be for fetchOne to return an  
empty collection ("no records") when it cannot find a record.  
However, that negates the convenience of fetchOne.

I'm not suggesting that, we use isNotFound or that we return a  
collection from fetchOne.  I'm suggesting that Records and Collections  
of records are different things and that attempting to unify their  
interfaces is treacherous in the sense of the Henry Spencer quote.

Just because you can do something, doesn't mean you should.


C. Meager Benefit II

The only example of concrete benefit that I've been provided has to do  
with converting the empty record into a new record.  In the setNames  
method of the Solar_Model_Tags_Collection class, there is a fragment  
of code that fetches a tag record and adds it if not found.

$tag = $model->fetchOneByName($name);
if ($tag->isEmpty()) {
        $tag->name = $name;
}
$this[] = $tag;

This represents a one line code savings over the current way of doing  
this:

$tag = $model->fetchOneByName($name);
if (!$tag) {
        $tag = $model->fetchNew();
        $tag->name = $name;
}
$this[] = $tag;

Which version do you find easier to understand?   Is it clear in the  
first example that a new record will be created in the db when you set  
the value of the name property?

I find this auto conversion of not found to new to be too magical and  
not in a good way.

Explicit is better.


D. Fail Fast

Because the record not found or no record indicator is actually a  
record, it will be harder to find errors.

If you attempt to get a property on a NULL value, you get a php error  
message.

If you attempt to set a property on a NULL value, you get a php error  
message.

if you attempt to call a method on a NULL value, you get a php error  
message.

If you attempt to get a property on an empty record, you get a value.

If you attempt to set a property on an empty record, you get a new  
record which can be saved in the DB.

If you attempt to call a method on an empty record, you get a  
potentially different behavior for each method?

Instead of failing early, the empty record change delays the detection  
of errors and an error may result in blank records being added to the  
DB.


E. Migration costs

Every test of a hasOne or BelongsTo property and every fetchOne test  
will have to be changed.

if (! $record)

Must be changed to

if (! $record->isEmpty())

The unfounded branch reverses the meaning of this test.  Since a  
record object will be returned where NULL used to be returned.

if (! $record) {
        // can never be executed if not changed to $record->isEmpty()
}

if ($record) {
        // Will always be executed if no changed to !$record->isEmpty()
}

Combined with delayed failure from D, converting an existing code base  
represents a significant undertaking.

Reviewing our code indicates that its very common to test the result  
of fetchOne and very common to test hasOne and BelongsTo properies for  
empty.

Its also hard to search for this type of usage.

Missing a test during migration is likely to result in an error  
because of the nature of the change.


F. Increased state management

Introducing an empty state into the Record object results in more  
overhead in tracking state for that object.

Since you cannot call a method on NULL, methods in classes descending  
from Record never have to worry about whether they are being called on  
an empty record.  After the unfounded branch, they do.

When you write methods on records, you have to take into account the  
impact of the empty state.


G.  Performance

Testing against null is faster than the isEmpty() function call.  
Creating Empty objects is slower and takes up more memory than using  
NULL.


To Summarize, I find the benefits of introducing an empty record  
instead of NULL to be meager and the drawbacks to be many.  The record  
oriented changes in the unfounded branch should be discarded.


COLLECTION CHANGES IN UNFOUNDED

The unfounded branch also changes how empty collections are  
represented.  Arrays are no longer used to represent collections in  
models.

fetchAll() will return a collection object, even if no records are  
found.

properties representing hasMany and hasManyThrough relationships will  
always have an collection object, even if there are no records in that  
collection.


A.  Results in less code

Its common to have operations on collections, but when arrays and  
collections are used interchangeably, there must be tests to convert  
between them.

if (!$member->applications) {
     $member->applications = $member->newRelated('applications');
}

$app = $member->applications->appendNew();

These guard clauses are annoying and are completely eliminated by the  
unfounded branch.  So only this line of code is needed:

$app = $member->applications->appendNew();

I think this is a clear win.

Special casing is not needed and you can always perform an operation  
on a collection.


B.  Least Surprise

Like with records, returning a collection from fetchAll() means that  
the standard php paradigm for testing against empty

if ($collection) {}

Returns the exact opposite of what one would expect.

This is unfortunate, but with the tools PHP allows, there is no  
perfect solution to what should be returned here.

A code review of the code available to me shows that it is relatively  
rare to perform this test on a collection or the result of a fetchAll.

It is far more common to pass the results immediately to foreach,  
which continues to work as expected.

So the benefits of the current way of returning array() to mean "no  
records" are not as large as I might have expected based on my code  
reviews.


C. Migration Cost

This change requires the same kind of migration as the record side  
would.

if (!$collection) {}

must be changed to

if ($collection->isEmpty()) {}

However, the patterns of usage on collections are very different.

The most common use of a result from fetchAll() or a hasMany property  
is to use foreach on it, which does not change.

The pattern in section A, where a property is tested and converted to  
a collection continues to work even if not migrated.

Reviewing our Model and Controller code, its uncommon in our code base  
to actually test a collection for empty using the php test for an  
empty array.

There is one more case which occurs in some view code:

if (!$list) {
        echo "none";
} else {
        echo "<li>";
        foreach($list as $item) { .. }
        echo "</ul>";
}

So making this change would technically break this code, but not with  
overly serious consequences.  Most changes are restricted to View code  
in our code base.

Additionally, that code should have probably accounted for the  
possibility of an empty collection being passed anyway.  Some of our  
existing code does do this by using count() to make this check.


I think the benefits of consistency and less code outweighs the  
drawbacks of migration costs for collections.  I think that the  
collection oriented changes in the unfounded branch should be merged  
to trunk.


So there is my official position on the unfounded branch.

Best Regards,

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

Hey Jeff -- thanks for summarizing all your previous criticisms into a  
single document.  I'll respond point-by-point later.  In the mean  
time, if you have examples of existing code that highlight your record-
related concerns, those would be a good addition to your commentary.




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jon Elofson]

Hi Guys,
I think Jeff raises some valid points regarding the new approach to
"not found" records. I'm looking forward to Paul's response to get a
better picture of the costs and benefits.

What would happen in a scenario like this, where I forget to alter my
code to isEmpty()? Could the following result in a new entry in my
table if no fields are required?
$record = $model->fetch($id);
if (!$record) {
    // but this won't happen now.
    return $this->_error("ERR_NO_SUCH_ITEM");
}
$data = $this->_request->post('record', array());
// Can you load into a non-existent record?
$record->load($data);
$record->updated_by = $this->user->auth->handle;

if ($this->_isProcess('save') && $record->save()) {
    // blah blah
}

There seems to be an assumption that I am going to want a new record
(a blank canvas, if you will) when I don't find the record I was
looking for. This might not be the case.

For me it's not too onerous to do the following:

if (!$item) {
       $item = $model->fetchNew();
       $item->property = $property;
}

And in this case, there are no assumptions. I want a new record.

Will the new approach have any significant increase to overhead?
Creating an object vs returning null.

Can a record be empty in the same way a collection can be? Isn't it
more a matter of existence?
To use an analogy. If I have a box of Wayne Gretzky hockey cards, then
I have a collection of records. If, however, my cards are stolen by
that weasel next door neighbor of mine, then I have an empty box; a
collection with no records, but the box still exists and has
properties. It's just empty.

If I look for a my Bobby Orr All Star card (a single record), but I
don't have one because my dad sold it for beer money, then do I have
an empty, blank card, or just no card at all?

Sorry for the hockey references (Canadian, eh). Maybe this analogy is
based on preconceived expectations. Like Jeff said, it's commonplace
to expect null or false when something isn't found.

Still, I am open to change. If the new approach is truly beneficial,
then I am all for it.

Jon

------------------

On Wed, Sep 30, 2009 at 7:09 PM, Paul M Jones <pmjones@...> wrote:

> Hey Jeff -- thanks for summarizing all your previous criticisms into a
> single document.  I'll respond point-by-point later.  In the mean
> time, if you have examples of existing code that highlight your record-
> related concerns, those would be a good addition to your commentary.
>
>
>
>
> --
>
> Paul M. Jones
> http://paul-m-jones.com/
>
>
>
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Richard Thomas-9]

Honestly I like both approaches, at times depending on what your doing  
either makes a lot of sense..

Maybe they both make enough sense that it should be a config option/
something you can pass as an option to fetch?

Default = as is right now for backwards compat.. And the overhead of  
another if statement would be so minimal compared to the overall  
benefit..

Matter fact, without a config option people are going to have to  
change simple if() checks to if(empty()) checks.. So the overhead  
saved of not having to do a empty() would outweigh the extra if  
statement needed to do this.


Richard Thomas
http://www.cyberlot.net




On Sep 30, 2009, at 11:08 PM, Jon Elofson wrote:

> Hi Guys,
> I think Jeff raises some valid points regarding the new approach to
> "not found" records. I'm looking forward to Paul's response to get a
> better picture of the costs and benefits.
>
> What would happen in a scenario like this, where I forget to alter my
> code to isEmpty()? Could the following result in a new entry in my
> table if no fields are required?
> $record = $model->fetch($id);
> if (!$record) {
>    // but this won't happen now.
>    return $this->_error("ERR_NO_SUCH_ITEM");
> }
> $data = $this->_request->post('record', array());
> // Can you load into a non-existent record?
> $record->load($data);
> $record->updated_by = $this->user->auth->handle;
>
> if ($this->_isProcess('save') && $record->save()) {
>    // blah blah
> }
>
> There seems to be an assumption that I am going to want a new record
> (a blank canvas, if you will) when I don't find the record I was
> looking for. This might not be the case.
>
> For me it's not too onerous to do the following:
>
> if (!$item) {
>       $item = $model->fetchNew();
>       $item->property = $property;
> }
>
> And in this case, there are no assumptions. I want a new record.
>
> Will the new approach have any significant increase to overhead?
> Creating an object vs returning null.
>
> Can a record be empty in the same way a collection can be? Isn't it
> more a matter of existence?
> To use an analogy. If I have a box of Wayne Gretzky hockey cards, then
> I have a collection of records. If, however, my cards are stolen by
> that weasel next door neighbor of mine, then I have an empty box; a
> collection with no records, but the box still exists and has
> properties. It's just empty.
>
> If I look for a my Bobby Orr All Star card (a single record), but I
> don't have one because my dad sold it for beer money, then do I have
> an empty, blank card, or just no card at all?
>
> Sorry for the hockey references (Canadian, eh). Maybe this analogy is
> based on preconceived expectations. Like Jeff said, it's commonplace
> to expect null or false when something isn't found.
>
> Still, I am open to change. If the new approach is truly beneficial,
> then I am all for it.
>
> Jon
>
> ------------------
>
> On Wed, Sep 30, 2009 at 7:09 PM, Paul M Jones <pmjones@...
> > wrote:
>> Hey Jeff -- thanks for summarizing all your previous criticisms  
>> into a
>> single document.  I'll respond point-by-point later.  In the mean
>> time, if you have examples of existing code that highlight your  
>> record-
>> related concerns, those would be a good addition to your commentary.
>>
>>
>>
>>
>> --
>>
>> Paul M. Jones
>> http://paul-m-jones.com/
>>
>>
>>
>>
>> _______________________________________________
>> Solar-talk mailing list
>> Solar-talk@...
>> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Richard Thomas-17]

If returning a record is preferred overall, Then make the current the  
default for now with a note in each release "fetch behavior  will  
change from returning a NULL to EMPTY RECORD in a future release,  
please update your configs if you don't want this behaviour"

Richard Thomas
http://www.phpjack.com

On Sep 30, 2009, at 11:23 PM, Richard Thomas wrote:

> Honestly I like both approaches, at times depending on what your  
> doing either makes a lot of sense..
>
> Maybe they both make enough sense that it should be a config option/
> something you can pass as an option to fetch?
>
> Default = as is right now for backwards compat.. And the overhead of  
> another if statement would be so minimal compared to the overall  
> benefit..
>
> Matter fact, without a config option people are going to have to  
> change simple if() checks to if(empty()) checks.. So the overhead  
> saved of not having to do a empty() would outweigh the extra if  
> statement needed to do this.
>
>
> Richard Thomas
> http://www.cyberlot.net
>
>
>
>
> On Sep 30, 2009, at 11:08 PM, Jon Elofson wrote:
>
>> Hi Guys,
>> I think Jeff raises some valid points regarding the new approach to
>> "not found" records. I'm looking forward to Paul's response to get a
>> better picture of the costs and benefits.
>>
>> What would happen in a scenario like this, where I forget to alter my
>> code to isEmpty()? Could the following result in a new entry in my
>> table if no fields are required?
>> $record = $model->fetch($id);
>> if (!$record) {
>>   // but this won't happen now.
>>   return $this->_error("ERR_NO_SUCH_ITEM");
>> }
>> $data = $this->_request->post('record', array());
>> // Can you load into a non-existent record?
>> $record->load($data);
>> $record->updated_by = $this->user->auth->handle;
>>
>> if ($this->_isProcess('save') && $record->save()) {
>>   // blah blah
>> }
>>
>> There seems to be an assumption that I am going to want a new record
>> (a blank canvas, if you will) when I don't find the record I was
>> looking for. This might not be the case.
>>
>> For me it's not too onerous to do the following:
>>
>> if (!$item) {
>>      $item = $model->fetchNew();
>>      $item->property = $property;
>> }
>>
>> And in this case, there are no assumptions. I want a new record.
>>
>> Will the new approach have any significant increase to overhead?
>> Creating an object vs returning null.
>>
>> Can a record be empty in the same way a collection can be? Isn't it
>> more a matter of existence?
>> To use an analogy. If I have a box of Wayne Gretzky hockey cards,  
>> then
>> I have a collection of records. If, however, my cards are stolen by
>> that weasel next door neighbor of mine, then I have an empty box; a
>> collection with no records, but the box still exists and has
>> properties. It's just empty.
>>
>> If I look for a my Bobby Orr All Star card (a single record), but I
>> don't have one because my dad sold it for beer money, then do I have
>> an empty, blank card, or just no card at all?
>>
>> Sorry for the hockey references (Canadian, eh). Maybe this analogy is
>> based on preconceived expectations. Like Jeff said, it's commonplace
>> to expect null or false when something isn't found.
>>
>> Still, I am open to change. If the new approach is truly beneficial,
>> then I am all for it.
>>
>> Jon
>>
>> ------------------
>>
>> On Wed, Sep 30, 2009 at 7:09 PM, Paul M Jones <pmjones@...
>> > wrote:
>>> Hey Jeff -- thanks for summarizing all your previous criticisms  
>>> into a
>>> single document.  I'll respond point-by-point later.  In the mean
>>> time, if you have examples of existing code that highlight your  
>>> record-
>>> related concerns, those would be a good addition to your commentary.
>>>
>>>
>>>
>>>
>>> --
>>>
>>> Paul M. Jones
>>> http://paul-m-jones.com/
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Solar-talk mailing list
>>> Solar-talk@...
>>> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>>>
>> _______________________________________________
>> Solar-talk mailing list
>> Solar-talk@...
>> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Sep 30, 2009, at 6:09 PM, Paul M Jones wrote:

> Hey Jeff -- thanks for summarizing all your previous criticisms into a
> single document.  I'll respond point-by-point later.  In the mean
> time, if you have examples of existing code that highlight your  
> record-
> related concerns, those would be a good addition to your commentary.

Paul,

Thanks in advance for taking the time to reply my concerns.  I know  
you're busy right now, but I mostly just wanted to collect all my  
arguments in one place and to gather some community feedback on some  
of the discussions we've had in private.

I'm not quite sure what kind of examples you're looking for.  Here is  
the data from my analysis on our code base.

Regarding the benefits of record changes...

We have 18 _belongsTo relationships in our model and 2 _hasOne  
relationships.

The auto-conversion of not found to new record is not relevant for  
_belongsTo because the pattern there is to have the record already and  
add something to a collection on that record.

I have exactly one case for our _hasOne relationships where we could  
use this feature:

             if (!$this->_area->config) {
                 $this->_area->config = $this->area_config->fetchNew();
                 $this->_area->config->area_id = $this->_area->id;
             }

However, I wouldn't use it because I think the following code is far  
less clear as to intent and makes it hard to grep for places where we  
create records:

             if (!$this->_area->config) {
                 $this->_area->config->area_id = $this->_area->id;
             }

Overall, I do not see any upside to the record changes.


Regarding the benefit of collection changes...

We have 7 calls to the deleteAll function on  
Solar_Sql_Model_Collection 1 call to removeOne that follow this form

         if ($this->items) {
             $this->items->deleteAll();
         }

With unfounded, this could be simplified to just this

             $this->items->deleteAll();

We have 15 calls to the appendNew function on  
Solar_Sql_Model_Collection.  However, only 3 of those calls are  
preceded by a test for an empty collection such as this...

             if (!$member->applications) {
                 $member->applications = $member-
 >newRelated('applications');
             }

             $app = $member->applications->appendNew();

So we have 3 cases where this would be simplified to

             $app = $member->applications->appendNew();

We have 14 cases where we call newRelated on a record.  In all 14  
cases, we are creating calling this on a toMany() relationship in  
order to set the collection.  10 are of this form:

                 if (!$this->member->keys) {
                     $this->member->keys = $this->member-
 >newRelated('keys');
                 }

And 4 are just naked collection creation:

         $layout->items = $layout->newRelated('items');

All of these would be obsolete in the unfounded branch.


So, for us, the total benefit of the collection portion of unfounded  
would be to eliminate 21 if tests and 4 naked calls to newRelated.

These checks are annoying and getting rid of them would be nice, but  
the benefits are actually fairly modest.


Now, looking at the cost of migration:

Doing a grep of ->fetchAll, ->fetchOne, and ->fetch( shows the  
following usages respectively.

fetchAll is used 66 times
fetchOne is used 76 times
fetch is used 201 times (not really, but with many things in solar  
named fetch, its hard to distinguish)

We would have to manually visit each of these and trace the result of  
the function calls through looking for places where we need to migrate  
to the unfounded paradigm.  There is no easy way to grep for this.

if (!$record)
if ($record)
$found = (bool) $model->fetch
if ($record = $model->fetchOne())

Now, moving to the points at which we access the properties  
representing a _hasOne or _belongsTo relationship.  These are very  
hard to grep for.  Gathering together the 20 names of our toOne  
relationships using this expression:

->(n1|n2|n3)(?!\w)

Gives 861 places where we may be dereferencing a toOne relationship  
property where we'd have to manually check to make sure it was  
properly handled afterwards.

On the collection side, we have 4 _hasManyThrough() relationships and  
16 _hasMany() relationships.  A similiar grep yields 244 places where  
we might be dereferencing a toMany relationship.

Keep in mind that since Solar_Struct and thus Solar_Sql_Model_Record  
allows you to access properties with array notation as well, we also  
have to look for this.  (I've been discouraging this use, but we have  
it.)  There may be some other alternative notations and conventions  
we'd also have to look out for.

A short review of these instances found via grep indicates that there  
is less impact for missing a migration change on collections than  
records.  For collections, everything we just make the change and  
everything will mostly work (because foreach is the same.)  The effort  
is in hunting down a few (i estimate a dozen or two) cases where thats  
not the case.  For Records, since the test is inverted every case  
where we do a test must be found and changed.

Based on this, I'd estimate that applying the collection changes would  
take about 1/4 the effort of applying both the record oriented changes  
and the collection changes.  That may be optimistic.

Either way, since this touches every part of our system, it involves a  
full QA sweep.  Thats probably a full week of effort by our QA guy.  I  
think I could change our code base for collections with 2 days of  
programming effort for just collections and probably 8 days of  
programming for the whole unfounded branch.

I'd say we couldn't digest the full unfounded branch in less than four  
weeks of elapsed time. (2 weeks code, 1 week intensive qa, 1 week  
follow up.)

The unfounded changes are expensive for what we get, but I think its  
worth it on the collection side changes because I think its the right  
thing to do and benefits Solar going forward.  (My boss would probably  
disagree.)  I think the record side changes are the wrong thing to do  
and hate to have to spend the time on it.  I think it will be bad for  
Solar in the long run because of the confusion factor as explained in  
my initial review.

I have 2 full work days already invested in analyzing the branch.

Is this the kind of information you were looking for paul?

Jeff

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Raymond Kolbe]

I guess I am not convinced that an empty record should be returned when
zero results are found using fetchOne().  I think Jon said it best with
his analogy.  An empty collection (the box) can exist, and things can be
added into that collection, but you either have a record or you don't.

As for Richard's suggestion for a config option, that doesn't sound like
a bad idea.  We already do this kind of thing with the HTTP request object:

// from Jon's example
$data = $this->_request->post('record', array());

Maybe we could do something like:

$record = $myModel->fetchOne($id, array('return' => false));

Can't wait to hear your response Paul ;-)

Thank you,
Ray Kolbe

Richard Thomas wrote:

> Honestly I like both approaches, at times depending on what your doing  
> either makes a lot of sense..
>
> Maybe they both make enough sense that it should be a config option/
> something you can pass as an option to fetch?
>
> Default = as is right now for backwards compat.. And the overhead of  
> another if statement would be so minimal compared to the overall  
> benefit..
>
> Matter fact, without a config option people are going to have to  
> change simple if() checks to if(empty()) checks.. So the overhead  
> saved of not having to do a empty() would outweigh the extra if  
> statement needed to do this.
>
>
> Richard Thomas
> http://www.cyberlot.net
>
>
>
>
> On Sep 30, 2009, at 11:08 PM, Jon Elofson wrote:
>
>  
>> Hi Guys,
>> I think Jeff raises some valid points regarding the new approach to
>> "not found" records. I'm looking forward to Paul's response to get a
>> better picture of the costs and benefits.
>>
>> What would happen in a scenario like this, where I forget to alter my
>> code to isEmpty()? Could the following result in a new entry in my
>> table if no fields are required?
>> $record = $model->fetch($id);
>> if (!$record) {
>>    // but this won't happen now.
>>    return $this->_error("ERR_NO_SUCH_ITEM");
>> }
>> $data = $this->_request->post('record', array());
>> // Can you load into a non-existent record?
>> $record->load($data);
>> $record->updated_by = $this->user->auth->handle;
>>
>> if ($this->_isProcess('save') && $record->save()) {
>>    // blah blah
>> }
>>
>> There seems to be an assumption that I am going to want a new record
>> (a blank canvas, if you will) when I don't find the record I was
>> looking for. This might not be the case.
>>
>> For me it's not too onerous to do the following:
>>
>> if (!$item) {
>>       $item = $model->fetchNew();
>>       $item->property = $property;
>> }
>>
>> And in this case, there are no assumptions. I want a new record.
>>
>> Will the new approach have any significant increase to overhead?
>> Creating an object vs returning null.
>>
>> Can a record be empty in the same way a collection can be? Isn't it
>> more a matter of existence?
>> To use an analogy. If I have a box of Wayne Gretzky hockey cards, then
>> I have a collection of records. If, however, my cards are stolen by
>> that weasel next door neighbor of mine, then I have an empty box; a
>> collection with no records, but the box still exists and has
>> properties. It's just empty.
>>
>> If I look for a my Bobby Orr All Star card (a single record), but I
>> don't have one because my dad sold it for beer money, then do I have
>> an empty, blank card, or just no card at all?
>>
>> Sorry for the hockey references (Canadian, eh). Maybe this analogy is
>> based on preconceived expectations. Like Jeff said, it's commonplace
>> to expect null or false when something isn't found.
>>
>> Still, I am open to change. If the new approach is truly beneficial,
>> then I am all for it.
>>
>> Jon
>>
>> ------------------
>>
>> On Wed, Sep 30, 2009 at 7:09 PM, Paul M Jones <pmjones@...
>>    
>>> wrote:
>>> Hey Jeff -- thanks for summarizing all your previous criticisms  
>>> into a
>>> single document.  I'll respond point-by-point later.  In the mean
>>> time, if you have examples of existing code that highlight your  
>>> record-
>>> related concerns, those would be a good addition to your commentary.
>>>
>>>
>>>
>>>
>>> --
>>>
>>> Paul M. Jones
>>> http://paul-m-jones.com/
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Solar-talk mailing list
>>> Solar-talk@...
>>> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>>>
>>>      
>> _______________________________________________
>> Solar-talk mailing list
>> Solar-talk@...
>> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>>    
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>  

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Steve Clay]

I'm jumping in the middle of this thread and don't have much hands on
experience w/ AR/TDG-ish classes* including Solar's yet, so please
discount my input accordingly...

The unified return type is certainly elegant to look at and probably
felt great to write, but I think it expands the purpose of the method
from "find" to "find (or new)". The next step down that road is to lose
fetchNew(); why should users learn it?

I think the current API (like Zend_Db_Table::fetchRow() which also
returns null) results in clearer user code. If the user wants to setup
magic to merge find and new it's easy enough...

At the very least the current docs should list null as a potential
return. http://www.solarphp.com/class/Solar_Sql_Model/fetchOne()

And because everyone loves analogies:
The DJ asked for a Buddy Holly 45 but you handed her a blank acetate:
"Here's *a* record, will this do?"

*If the goal of Solar_Sql_Model is to emulate a popular existing API
with this convention, then this changes everything.

Cheers,
--
Steve Clay
http://mrclay.org/
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Oct 1, 2009, at 5:54 PM, Raymond Kolbe wrote:

> Maybe we could do something like:
>
> $record = $myModel->fetchOne($id, array('return' => false));

I'd be ok with this as a parameter to fetchOne() and fetch(), but with  
a default that preserves backward compatibility.  I wouldn't find that  
very useful, but if others would, It wouldn't matter to me.

This does not resolve what should be stored in the property  
representing a hasOne or BelongsTo relationship when there is no  
related record.  I think that should be null.

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

Hi everyone,

First off, I found Jeff's review to be highly valuable, in that it  
represents a different view with a modified approach, but a similar  
goal: stable, predictable code behaviors.  Thanks again, Jeff, for  
taking the time to write it up.  In summary: Jeff is in favor of  
discarding the record changes, but keeping the collection changes, in  
branches/unfounded.

To be clear, I am not ideologically committed to the changes in  
branches/unfounded. They represent an experimental set of features,  
some of which may be retained, others of which may be discarded.

I find most of Jeff's arguments against the record changes can be used  
against the collection changes.  As an example, Jeff says this in the  
intro portion of "Record Changes":

 > This requires that code of this form
 >
 > $item = $model->fetchOne();
 > if (! $item) {
 > ...
 > }
 >
 > Must be changed to
 >
 > $item = $model->fetchOne();
 > if ($item->isEmpty()) {
 > ...
 > }
 >
 > Additionally, in models any hasOne or BelongsTo relationship will  
also
 > change.  Instead of the property containing NULL when there is no
 > record, the property will contain an "Empty Record" to indicate that
 > there is no record in the relationship.


A search-and-replace using Collection terminology results in this  
equally-true narrative:

 > This requires that code of this form
 >
 > $list = $model->fetchAll();
 > if (! $list) {
 > ...
 > }
 >
 > Must be changed to
 >
 > $list = $model->fetchAll();
 > if ($list->isEmpty()) {
 > ...
 > }
 >
 > Additionally, in models any hasMany or hasManyThrough relationship  
will also
 > change.  Instead of the property containing array() when there is no
 > collection, the property will contain an "Empty Collection" to  
indicate that
 > there is no collection for the relationship.


A "least-astonishment" search-and-replace:

 > Receiving a collection from fetchAll() where no collection should  
exists is
 > "Astonishing," and not not in a good way.


A "migration costs" search-and-replace:

 > Every test of a hasMany or hasManyThrough property and every  
fetchAll test
 > will have to be changed.
 >
 > if (! $collection)
 >
 > Must be changed to
 >
 > if (! $collection->isEmpty())
 >
 > The unfounded branch reverses the meaning of this test.  Since a
 > collection object will be returned where array() used to be returned.
 >
 > if (! $collection) {
 > // can never be executed if not changed to $collection->isEmpty()
 > }
 >
 > if ($collection) {
 > // Will always be executed if no changed to !$collection->isEmpty()
 > }


A "performance" search-and-replace:

 > Testing against array() is faster than the isEmpty() function call.
 > Creating Empty objects is slower and takes up more memory than using
 > array().


Etc., et al., and so on. Yes, records and collections are "different"  
from each other; the question is, are they different *enough* to  
warrant Jeff's preferences?  Honestly, I'm not yet certain.  I do feel  
those particular arguments are weak in comparison to other points in  
Jeff's review; it is to those stronger arguments I will respond below.


* * *

 > B. Meager Benefit I
 >
 > I'm told the benefit to changing how not found is processed is to  
make
 > the interface between Records and Collections similar.

Not exactly; the intended benefit is to remove the need for the  
following idiom in records where you need to access a related object:

     // where 'foo' is a related record or collection ...
     if (! $this->foo) {
         $this->foo = $this->newRelated('foo');
     }
     // ... now you can be sure that $this->foo is actually there

Every time you need 'foo', you need to check if it's really there or  
not. This is true for both records and collections. How much better  
would it be to not have to that every time you need a related 'foo'? A  
lot better, I think, but I could be wrong.

 > Attempting to represent "no record" with a special record
 > that has the same interface as an empty collection ("no records") is
 > only going to lead to confusion.  If you are confused by that
 > sentence, you are proving my point.

Consider that an empty collection currently being represented by an  
array(), and a not-found record currently being represented by a null,  
can be identified as such:

     if (empty($record)) { ... }
     if (empty($collection)) { ... }

As such, it seems to map closely that using objects marked as empty  
should also have similar interfaces:

     if ($record->isEmpty()) { ... }
     if ($collection->isEmpty()) { ... }

I find it hard to believe that either of those is overwhelmingly  
"confusing", but then, I might be too close to the problem to see it  
the way others would.

As a corollary, using null for record, but a collection object marked  
as empty, results in this:

     if (! $record) { ... }
     if ($collection->isEmpty()) { ... }

The lack of consistency there bothers me. That records and collections  
are "different" we know; the question is, are they different *enough*  
to warrant different idioms and interfaces in this case?


* * *

 > C. Meager Benefit II
 >
 > The only example of concrete benefit that I've been provided has to  
do
 > with converting the empty record into a new record.

Not entirely accurate; I spoke with Jeff about the idiom I noted  
above ...

     // where 'foo' is a related record or collection ...
     if (! $this->foo) {
         $this->foo = $this->newRelated('foo');
     }
     // ... now you can be sure that $this->foo is actually there

... and he does in fact note it as a concrete benefit in "Collection  
Changes" under "A. Results in less code" The point is that this idiom  
applies to records *as well as* collections.

Regarding the conversion of an "empty" record to a "new" one, consider  
the collection object in branches/unfounded. If you get "no records"  
in a collection (an empty state), you can append to it, and the new  
records will be saved.

     $collection = $model->fetchAll();
     if ($collection->isEmpty()) {
         $collection->appendNew(array(...));
     }
     $collection->save();

The idea with converting an "empty" record to "new" is to mimic that  
behavior.

     $record = $model->fetchOne();
     if ($record->isEmpty()) {
         $record->foo = 'bar';
     }
     $record->save();

It would be possible to abandon that particular behavior, and make  
"empty" records more inert. That would mean no conversion-to-new, and  
any use of __get() or __set() would throw an exception. The resulting  
code example would be this:

     $record = $model->fetchOne();
     if ($record->isEmpty()) {
         $record = $model->fetchNew();
         $record->foo = 'bar';
     }
     $record->save();

However, that leads back to a variation on original idiom I wanted to  
eliminate, but only for records:

     // where 'foo' is a related record ...
     if ($this->foo->isEmpty()) {
         $this->foo = $this->newRelated('foo');
     }
     // ... now you can be sure that $this->foo is actually there

That becomes a second inconsistency; you have to check for the  
existence of records (and create the new related record), but not for  
collections. This bothers me.


* * *

 > D. Fail Fast
...
 > Instead of failing early, the empty record change delays the  
detection
 > of errors and an error may result in blank records being added to the
 > DB.

... only if you set values on them.  Records marked as empty don't get  
saved
to the database.


* * *

Regarding collections, I agree with Jeff on every single point.  The  
collection changes are something I really like; the question now is,  
do records deserve similar treatment, if only for consistency's sake?  
I believe they do, if it can be done without screwing up other stuff.

I want to reiterate that even after having said all this, it may yet  
turn out that "empty record object" is dumb for all the reasons Jeff  
notes, perhaps even in addition to other reasons not yet discovered. I  
am not ideologically committed to the empty record object yet, and am  
not willing to commit them to branches/beta at this point.  There may  
simply have to be some level of perceived inconsistency, because of  
different use-patterns between records and collections.

I have gone so far as to create branches/nullrec as an exploration of  
keeping the collection changes, and abandon the record changes.  You  
all may wish to review that to see the differences between the two  
approaches.

Questions?  Comments?


--

Paul M. Jones
http://paul-m-jones.com/

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jon Elofson]

I like the idea of consistency in how one deals with records and
collections; however, I am still not convinced that they are similar
enough to warrant they same automatic creation. Automatic creation of
a record when one isn't there seems most useful as a convenience and
makes an assumption that a new record should be created; a sort of,
implied consent. You make a good point about empty($item) and
empty($list). Still, false and empty mean completely two different
things to me, so maybe that is more of a problem with PHP's empty()
than anything else.

As I mentioned before, my opinion on this is partially based on
expectation. Compare PDO::fetch() vs PDO:: fetchAll(). If fetch
doesn't find anything, it returns false, while fetchAll will return an
empty array. I know these are not the same as records and collections,
per se, but the comparison is valid in what one expects to be
returned. As convenient as it may be, especially went working with
related records, I still feel that a record should not automatically
be created when it's not there. Just my two cents, though.

Jon

On Fri, Oct 2, 2009 at 5:03 PM, Paul M Jones <pmjones@...> wrote:

> Hi everyone,
>
> First off, I found Jeff's review to be highly valuable, in that it
> represents a different view with a modified approach, but a similar
> goal: stable, predictable code behaviors.  Thanks again, Jeff, for
> taking the time to write it up.  In summary: Jeff is in favor of
> discarding the record changes, but keeping the collection changes, in
> branches/unfounded.
>
> To be clear, I am not ideologically committed to the changes in
> branches/unfounded. They represent an experimental set of features,
> some of which may be retained, others of which may be discarded.
>
> I find most of Jeff's arguments against the record changes can be used
> against the collection changes.  As an example, Jeff says this in the
> intro portion of "Record Changes":
>
>  > This requires that code of this form
>  >
>  > $item = $model->fetchOne();
>  > if (! $item) {
>  >      ...
>  > }
>  >
>  > Must be changed to
>  >
>  > $item = $model->fetchOne();
>  > if ($item->isEmpty()) {
>  >      ...
>  > }
>  >
>  > Additionally, in models any hasOne or BelongsTo relationship will
> also
>  > change.  Instead of the property containing NULL when there is no
>  > record, the property will contain an "Empty Record" to indicate that
>  > there is no record in the relationship.
>
>
> A search-and-replace using Collection terminology results in this
> equally-true narrative:
>
>  > This requires that code of this form
>  >
>  > $list = $model->fetchAll();
>  > if (! $list) {
>  >      ...
>  > }
>  >
>  > Must be changed to
>  >
>  > $list = $model->fetchAll();
>  > if ($list->isEmpty()) {
>  >      ...
>  > }
>  >
>  > Additionally, in models any hasMany or hasManyThrough relationship
> will also
>  > change.  Instead of the property containing array() when there is no
>  > collection, the property will contain an "Empty Collection" to
> indicate that
>  > there is no collection for the relationship.
>
>
> A "least-astonishment" search-and-replace:
>
>  > Receiving a collection from fetchAll() where no collection should
> exists is
>  > "Astonishing," and not not in a good way.
>
>
> A "migration costs" search-and-replace:
>
>  > Every test of a hasMany or hasManyThrough property and every
> fetchAll test
>  > will have to be changed.
>  >
>  > if (! $collection)
>  >
>  > Must be changed to
>  >
>  > if (! $collection->isEmpty())
>  >
>  > The unfounded branch reverses the meaning of this test.  Since a
>  > collection object will be returned where array() used to be returned.
>  >
>  > if (! $collection) {
>  >      // can never be executed if not changed to $collection->isEmpty()
>  > }
>  >
>  > if ($collection) {
>  >      // Will always be executed if no changed to !$collection->isEmpty()
>  > }
>
>
> A "performance" search-and-replace:
>
>  > Testing against array() is faster than the isEmpty() function call.
>  > Creating Empty objects is slower and takes up more memory than using
>  > array().
>
>
> Etc., et al., and so on. Yes, records and collections are "different"
> from each other; the question is, are they different *enough* to
> warrant Jeff's preferences?  Honestly, I'm not yet certain.  I do feel
> those particular arguments are weak in comparison to other points in
> Jeff's review; it is to those stronger arguments I will respond below.
>
>
> * * *
>
>  > B. Meager Benefit I
>  >
>  > I'm told the benefit to changing how not found is processed is to
> make
>  > the interface between Records and Collections similar.
>
> Not exactly; the intended benefit is to remove the need for the
> following idiom in records where you need to access a related object:
>
>     // where 'foo' is a related record or collection ...
>     if (! $this->foo) {
>         $this->foo = $this->newRelated('foo');
>     }
>     // ... now you can be sure that $this->foo is actually there
>
> Every time you need 'foo', you need to check if it's really there or
> not. This is true for both records and collections. How much better
> would it be to not have to that every time you need a related 'foo'? A
> lot better, I think, but I could be wrong.
>
>  > Attempting to represent "no record" with a special record
>  > that has the same interface as an empty collection ("no records") is
>  > only going to lead to confusion.  If you are confused by that
>  > sentence, you are proving my point.
>
> Consider that an empty collection currently being represented by an
> array(), and a not-found record currently being represented by a null,
> can be identified as such:
>
>     if (empty($record)) { ... }
>     if (empty($collection)) { ... }
>
> As such, it seems to map closely that using objects marked as empty
> should also have similar interfaces:
>
>     if ($record->isEmpty()) { ... }
>     if ($collection->isEmpty()) { ... }
>
> I find it hard to believe that either of those is overwhelmingly
> "confusing", but then, I might be too close to the problem to see it
> the way others would.
>
> As a corollary, using null for record, but a collection object marked
> as empty, results in this:
>
>     if (! $record) { ... }
>     if ($collection->isEmpty()) { ... }
>
> The lack of consistency there bothers me. That records and collections
> are "different" we know; the question is, are they different *enough*
> to warrant different idioms and interfaces in this case?
>
>
> * * *
>
>  > C. Meager Benefit II
>  >
>  > The only example of concrete benefit that I've been provided has to
> do
>  > with converting the empty record into a new record.
>
> Not entirely accurate; I spoke with Jeff about the idiom I noted
> above ...
>
>     // where 'foo' is a related record or collection ...
>     if (! $this->foo) {
>         $this->foo = $this->newRelated('foo');
>     }
>     // ... now you can be sure that $this->foo is actually there
>
> ... and he does in fact note it as a concrete benefit in "Collection
> Changes" under "A. Results in less code" The point is that this idiom
> applies to records *as well as* collections.
>
> Regarding the conversion of an "empty" record to a "new" one, consider
> the collection object in branches/unfounded. If you get "no records"
> in a collection (an empty state), you can append to it, and the new
> records will be saved.
>
>     $collection = $model->fetchAll();
>     if ($collection->isEmpty()) {
>         $collection->appendNew(array(...));
>     }
>     $collection->save();
>
> The idea with converting an "empty" record to "new" is to mimic that
> behavior.
>
>     $record = $model->fetchOne();
>     if ($record->isEmpty()) {
>         $record->foo = 'bar';
>     }
>     $record->save();
>
> It would be possible to abandon that particular behavior, and make
> "empty" records more inert. That would mean no conversion-to-new, and
> any use of __get() or __set() would throw an exception. The resulting
> code example would be this:
>
>     $record = $model->fetchOne();
>     if ($record->isEmpty()) {
>         $record = $model->fetchNew();
>         $record->foo = 'bar';
>     }
>     $record->save();
>
> However, that leads back to a variation on original idiom I wanted to
> eliminate, but only for records:
>
>     // where 'foo' is a related record ...
>     if ($this->foo->isEmpty()) {
>         $this->foo = $this->newRelated('foo');
>     }
>     // ... now you can be sure that $this->foo is actually there
>
> That becomes a second inconsistency; you have to check for the
> existence of records (and create the new related record), but not for
> collections. This bothers me.
>
>
> * * *
>
>  > D. Fail Fast
> ...
>  > Instead of failing early, the empty record change delays the
> detection
>  > of errors and an error may result in blank records being added to the
>  > DB.
>
> ... only if you set values on them.  Records marked as empty don't get
> saved
> to the database.
>
>
> * * *
>
> Regarding collections, I agree with Jeff on every single point.  The
> collection changes are something I really like; the question now is,
> do records deserve similar treatment, if only for consistency's sake?
> I believe they do, if it can be done without screwing up other stuff.
>
> I want to reiterate that even after having said all this, it may yet
> turn out that "empty record object" is dumb for all the reasons Jeff
> notes, perhaps even in addition to other reasons not yet discovered. I
> am not ideologically committed to the empty record object yet, and am
> not willing to commit them to branches/beta at this point.  There may
> simply have to be some level of perceived inconsistency, because of
> different use-patterns between records and collections.
>
> I have gone so far as to create branches/nullrec as an exploration of
> keeping the collection changes, and abandon the record changes.  You
> all may wish to review that to see the differences between the two
> approaches.
>
> Questions?  Comments?
>
>
> --
>
> Paul M. Jones
> http://paul-m-jones.com/
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Oct 4, 2009, at 15:08 , Jon Elofson wrote:

> As I mentioned before, my opinion on this is partially based on
> expectation. Compare PDO::fetch() vs PDO:: fetchAll(). If fetch
> doesn't find anything, it returns false, while fetchAll will return an
> empty array. I know these are not the same as records and collections,
> per se, but the comparison is valid in what one expects to be
> returned.

I think that is the best argument in favor of "different enough" that  
I've heard yet.  Thanks Jon.  :-)



--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jon Elofson]

For what it's worth, I do see why auto creation of the record would be
handy. I'll be happy with whatever direction you decide on.

I like seeing all the discussion and development going on lately too.
It's good stuff.

Jon

On 10/4/09, Paul M Jones <pmjones@...> wrote:

> On Oct 4, 2009, at 15:08 , Jon Elofson wrote:
>
>> As I mentioned before, my opinion on this is partially based on
>> expectation. Compare PDO::fetch() vs PDO:: fetchAll(). If fetch
>> doesn't find anything, it returns false, while fetchAll will return an
>> empty array. I know these are not the same as records and collections,
>> per se, but the comparison is valid in what one expects to be
>> returned.
>
> I think that is the best argument in favor of "different enough" that
> I've heard yet.  Thanks Jon.  :-)
>
>
>
> --
>
> Paul M. Jones
> http://paul-m-jones.com/
>
>
>
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Oct 2, 2009, at 4:03 PM, Paul M Jones wrote:

> I find most of Jeff's arguments against the record changes can be used
> against the collection changes.  As an example, Jeff says this in the
> intro portion of "Record Changes":

I absolutely agree with this.  That's why I think the changes to  
Collections are only modestly beneficial.

Because of the way PHP handles empty and Objects that implement  
Iterator, there can be no perfect solution to this issue in current PHP.

For now, we have to pick the places where we'd like our solution to be  
sub-optimal.

> Not exactly; the intended benefit is to remove the need for the
> following idiom in records where you need to access a related object:
>
>     // where 'foo' is a related record or collection ...
>     if (! $this->foo) {
>         $this->foo = $this->newRelated('foo');
>     }
>     // ... now you can be sure that $this->foo is actually there
>
> Every time you need 'foo', you need to check if it's really there or
> not. This is true for both records and collections. How much better
> would it be to not have to that every time you need a related 'foo'? A
> lot better, I think, but I could be wrong.

...
> That becomes a second inconsistency; you have to check for the
> existence of records (and create the new related record), but not for
> collections. This bothers me.


There is a very big distinction with this paradigm between collections  
and records.

The test in the code above when used on a record above CREATES a new  
record.

The test in the code above when used on a collection CONVERTS the  
representation of "no records" from an empty array to an empty  
collection.  Nothing is created.

I think this is an important distinction.  They really are different  
operations.  I think the conversion operation should be eliminated.  I  
think the creation operation should remain explicit.

All the examples I've been given of the advantages of the record  
conversion hinge on the records creation example.  I'd like to see an  
example of benefit that does not involve record creation.

Thanks for your consideration, response and the null-record branch.

Best Regards,

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Cruz, Darwin]

Working off an Oracle 10G Database.
PHP 5.2.10

Is it possible to modify the character set in the PDO connection when
generating a query to insert/update data to the database?  The reason
I'm asking is because I am trying to insert into a CLOB column using
multi-byte charset (AL32UTF8)
results on the following error:

ORA-01461: can bind a LONG value only for insert into a LONG column.

The column datatype in not LONG it's a CLOB.

Changing the connection charset to use WE8ISO8859P1 or any other single
byte charset should solve the problem.

I'm looking at the Solar_Sql_Adapter class connect() function and the
_prepare().  I'm wondering which would be the best place to try this.

Thanks!
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Cruz, Darwin]

In the Solar_Sql_Adapter connect() method I tried the following and I'm
still getting the ORA-01461: can bind a LONG value only for insert into
a LONG column exception:


    public function connect()
    {
        // if we already have a PDO object, no need to re-connect.
        if ($this->_pdo) {
            return;
        }
       
        // start profile time
        $time = microtime(true);
        $dsn = $this->_dsn.";charset=WE8ISO8859P1";
        // attempt the connection
        $this->_pdo = new PDO(
            $dsn,
            $this->_config['user'],
            $this->_config['pass']
        );
        // post-connection tasks
        $this->_postConnect();
       
        // retain the profile data?
        $this->_addProfile($time, '__CONNECT');
    }


I've been able to insert into the CLOBS using PHP OCI8 extension but no
luck with the PDO.  Example for using OCI8 is found here:
http://www.oracle.com/technology/pub/articles/oracle_php_cookbook/fuecks
_lobs.html  




-----Original Message-----
From: solar-talk-bounces@...
[mailto:solar-talk-bounces@...] On Behalf Of Cruz, Darwin
Sent: Monday, October 05, 2009 2:48 PM
To: solar-talk@...
Subject: [Solar-talk] How can i modify the database character set on the
PDOobject

Working off an Oracle 10G Database.
PHP 5.2.10

Is it possible to modify the character set in the PDO connection when
generating a query to insert/update data to the database?  The reason
I'm asking is because I am trying to insert into a CLOB column using
multi-byte charset (AL32UTF8)
results on the following error:

ORA-01461: can bind a LONG value only for insert into a LONG column.

The column datatype in not LONG it's a CLOB.

Changing the connection charset to use WE8ISO8859P1 or any other single
byte charset should solve the problem.

I'm looking at the Solar_Sql_Adapter class connect() function and the
_prepare().  I'm wondering which would be the best place to try this.

Thanks!
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Antti Holvikari-2]

I'm +1 for the collection->isEmpty() but -1 for record->isEmpty().
While I love the consistency of related objects to always have
isEmpty(), I think null is more close to "no record" than an empty
record. "no record" is *nothing*, while an empty list still should
behave like a list.

--
-antti
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

Hi all,

The consensus appears to be in favor of "empty collection" and against  
"empty record".  Believe it or not, I'm kind of OK with that.

However, I would like to suggest something that only recently occurred  
to me; perhaps this will find favor.


* * *

To repeat from earlier in this thread, the intended benefit of "empty  
record/collection" was to remove the need for the following idiom in  
records where you need to access a related object:

     // where 'foo' is a related record or collection ...
     if (! $this->foo) {
         $this->foo = $this->newRelated('foo');
     }
     // ... now you can be sure that $this->foo is actually there

Every time you need the related 'foo', you need to check if it's  
really there or not. This is true for both records and collections.

That is the idiom I very much want to get rid of.  It's really under  
my skin.


* * *

It struck me recently that the use case I'm talking about happens  
always and only within record objects.

What say if the "empty" objects are used *only* within a record; i.e.,  
with related records and collections on a record object?  The idea  
would be that a normal fetch*() call returns null or array as before ...

     $item = $model->foos->fetchOne(); // null
     $list = $model->foos->fetchAll(); // array()

... but *related* elements on a record get the "empty object" treatment:

     $item = $model->foos->fetchOne(); // a found record
     $item->bars; // an empty collection object
     $item->baz; // an empty record object

That addresses directly the particular idiom I'd like to remove.  (It  
does introduce an inconsistency between a regular fetch result and a  
related result, but I think the different contexts of "inside the  
record object" and "outside the record object" would be enough of a  
distinguisher.)


* * *

The next problem is, what should the empty related objects look like?  
I think we're all satisfied with the empty collection object and its  
behaviors.  It's the empty record that is the tough one.  The empty  
record looks like a regular record in a lot of respects, but the  
methods and properties behave differently depending on circumstances,  
and so it's misleading.

Perhaps instead of an "empty record" as implemented in branches/
unfounded, the related placeholder records could be actual *new*  
records instead.  The new record might have a flag on it ("empty" or  
"not found") to indicate it started life as a placeholder for a  
related record that didn't exist, but it would otherwise be a regular  
new record from the related model fetchNew() with no other special  
behaviors.


* * *

Finally, we have the "save" behavior for these hypothetical new-but-
empty related record placeholders.  There are two options that I can  
see here:

     1. Always save the placeholder record no matter what (creating to-
one relateds and possibly cascading stuff down the line); or

     2. Only save the placeholder record if there have been some  
changes to it (or some other indicator to force the save).

I'd say the latter behavior is more-correct; that is, if the  
placeholder "new" record has not changed in some way, it should not  
auto-save itself.   (If "always save" is needed, I think a  
postSaveRelated() hook to do so would be an easy thing to implement on  
a per-model basis.)


* * *

Thoughts?




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

Picking up from a conversation of old...

On Oct 14, 2009, at 6:14 PM, Paul M Jones wrote:

> To repeat from earlier in this thread, the intended benefit of "empty  
> record/collection" was to remove the need for the following idiom in  
> records where you need to access a related object:
>
>     // where 'foo' is a related record or collection ...
>     if (! $this->foo) {
>         $this->foo = $this->newRelated('foo');
>     }
>     // ... now you can be sure that $this->foo is actually there
>
> Every time you need the related 'foo', you need to check if it's  
> really there or not. This is true for both records and collections.
>
> That is the idiom I very much want to get rid of.  It's really under  
> my skin.

I agree, having to convert representations is low value code.

> It struck me recently that the use case I'm talking about happens  
> always and only within record objects.
>
> What say if the "empty" objects are used *only* within a record; i.e.,  
> with related records and collections on a record object?  The idea  
> would be that a normal fetch*() call returns null or array as before ...
>
>     $item = $model->foos->fetchOne(); // null
>     $list = $model->foos->fetchAll(); // array()
>
> ... but *related* elements on a record get the "empty object" treatment:
>
>     $item = $model->foos->fetchOne(); // a found record
>     $item->bars; // an empty collection object
>     $item->baz; // an empty record object

I'm ok with doing this for collections, but not for records.

    if (! $this->foos) {
        $this->foos = $this->newRelated('foos');
    }

for a collection this is just a simple conversion between empty array and empty collection.  You could freely convert back and forth between the two.  The only reason to prefer the collection is to be able to make method calls on the collection.  Note, this works in trunk today.

I could just as easily do this:

if (count($this->foos) == 0) {
    $this->foos = array();
}

To go in the opposite direction.

Now lets say that I execute this code in trunk today on a toMany relationship:

    if (! $this->foos) {
        $this->foos = $this->newRelated('foos');
    }

Any down stream code MUST be prepared to deal with the fact that there is an empty collection representing the relationship.  any code that does not prepare for the possibility that $record->foo can be an empty collection instead of an empty array is probably buggy.

Thus, changing from array() to empty collection as the default representation for empty for toMany relationships is a relatively minor change.  Any bugs are not new bugs introduced, they are existing bugs revealed.

This is NOT the case on the record side.  There is no concept of an empty record in trunk.  That is completely new.  for a toOne relationship this code is not a simple representational conversion:

    if (! $this->foo) {
        $this->foo = $this->newRelated('foo');
    }

Although the notation is the same, the outcome is different, creating a new record instead of converting between equivelent representations.

I think the problem is that the newRelated function has two different outcomes, two different return values, record or collection.  

newRelated is really a shortcut for

$model->getRelated('foos')->getModel()->newCollection();

or

$model->getRelated('foo')->getModel()->fetchNew();

So, making relationships always have collections would eliminate the need for the first form.

As far as the second form, I don't find it a problem at all to say fetchNew when you mean to create a new record.

I've stated before all my objections to empty record.  I don't see anything in this proposal that changes that.

So, no different than my last opinion.  :)

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Oct 14, 2009, at 6:14 PM, Paul M Jones wrote:

> To repeat from earlier in this thread, the intended benefit of "empty  
> record/collection" was to remove the need for the following idiom in  
> records where you need to access a related object:
>
>     // where 'foo' is a related record or collection ...
>     if (! $this->foo) {
>         $this->foo = $this->newRelated('foo');
>     }

Just for emphasis, this could be rewritten as

// If foos is not a collection, convert it to a collection so we can call methods on it
if (!($record->foos instanceof Solar_Sql_Model_Collection)) {
        $record->foos = $record->getModel()->newRelated('foos', $record->foos);
}

By all means, get rid of the need to do that.

However

// If there is no foo record, create one
if (!$record->foo) {
        $record->foo = $record->getModel()->newRelated('foo');
}
$record->save();

Is fine in my book.

Also, note that this only makes sense for hasOne relationships.  Because of the way that save cascades if you try to use this notation with a BelongsTo relationship, the record created doesn't save.

I just don't see this idiom as onerous for hasOne relationships and records and you'd never use it for BelongsTo.

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk