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

Timestamp behavior follow datetimetype set immutable #11586

Merged
merged 7 commits into from Jan 2, 2018

Conversation

Projects
None yet
7 participants
@o0h
Contributor

o0h commented Dec 30, 2017

TimestampBehavior hard construct Cake\I18n\Time instance. (here)
So, when new entity instance got by Table->save created(or modified) property is Time (new Time).
But with useImmutable, the entity got by Table->get created property is FrozenTime (DateTimeType->toPHP()).

The same properties of same entity should be same type wherever they come from.


What you did

$Table = TableRegistry::get('Users');
$entity = $Table->newEntity(['name' => 'my name']);
$saved = $Table->save($entity);
$read = $Table->get($entity->id);

echo get_class($saved->created); // Cake\I18n\Time
echo get_class($read->created); //  Cake\I18n\FrozenTime

What you expected to happen

-- echo get_class($saved); // Cake\I18n\Time
++ echo get_class($saved); // Cake\I18n\FrozenTime
echo get_class($read); //  Cake\I18n\FrozenTime
@codecov-io

This comment has been minimized.

codecov-io commented Dec 30, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (3.next@acb2ce9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             3.next   #11586   +/-   ##
=========================================
  Coverage          ?   93.36%           
  Complexity        ?    13011           
=========================================
  Files             ?      436           
  Lines             ?    32830           
  Branches          ?        0           
=========================================
  Hits              ?    30651           
  Misses            ?     2179           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
src/ORM/Behavior/TimestampBehavior.php 97.77% <100%> (ø) 22 <5> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acb2ce9...5c21e5f. Read the comment docs.

@lorenzo

This comment has been minimized.

Member

lorenzo commented Dec 30, 2017

Looks good to me

@dereuromark

This comment has been minimized.

Member

dereuromark commented Dec 30, 2017

Given the typehint changes this sounds like 3.6 changes.
People could have extended the class.

*/
public function timestamp(DateTime $ts = null, $refreshTimestamp = false)
public function timestamp(DateTimeInterface $ts = null, $refreshTimestamp = false)

This comment has been minimized.

@chinpei215

chinpei215 Dec 30, 2017

Member

Strictly speaking, we cannot change signatures in bugfix releases. It may be better to change from master to 3.next.

{
/** @var \Cake\Database\Type\DateTimeType $type */
$type = Type::build('datetime');

This comment has been minimized.

@chinpei215

chinpei215 Dec 30, 2017

Member

It is better than now, but if the modified field is declared as timestamp, and the Type settings are different between 'datetime' and 'timestamp', I think it doesn't work as expected.

See: https://github.com/cakephp/app/blob/94f120be6498f05654dfa83f6ea79302fa667661/config/bootstrap.php#L185-L188

This comment has been minimized.

@o0h

o0h Dec 30, 2017

Contributor

it slipped my mind..

as default of Type::$_types, datetime and timestamp both are bound to DateTimeType.
(and DateTimeType::$_className value is shared as statically.)

i should consider there are difference between created and modified column types ?

This comment has been minimized.

@chinpei215

chinpei215 Dec 30, 2017

Member

Hmm. It seems impossible to detect correct types in this method, as it doesn't know what field uses the timestamp. I think it can be done in the _updateField() method instead, by using $this->_table->getSchema().

This comment has been minimized.

@markstory

markstory Dec 30, 2017

Member

We won't be able to get the class correct 100% of the time, but it is more correct now than it was in the past.

This comment has been minimized.

@lorenzo

lorenzo Dec 30, 2017

Member

It is true that should plan for a change that actually uses the right type

This comment has been minimized.

@markstory

markstory Dec 30, 2017

Member

How would we get the correct types based on the arguments to this method?

This comment has been minimized.

@o0h

o0h Dec 31, 2017

Contributor

can i add field option to timestamp() and _updateField() passes field name ?


    /**
     * @param string  $columnType 
     */
    public function timestamp(DateTimeInterface $ts = null, $refreshTimestamp = false, $columnType = 'datetime')
    {
        $type = Type::build($columnType);
        $class = $type->getDateTimeClassName();

and

    protected function _updateField($entity, $field, $refreshTimestamp)
    {
        if ($entity->isDirty($field)) {
            return;
        }
        $columnType = $this->getTable()->getScheme()->getColumnType($field);
        $entity->set($field, $this->timestamp(null, $refreshTimestamp, $columnType));
    }

This comment has been minimized.

@o0h

o0h Dec 31, 2017

Contributor

if we should use the right type, maybe it would be better to to keep $_ts each fields...
because when $ts is null && !$refreshTimestamp, timestamp() returns cached instance.

This comment has been minimized.

@chinpei215

chinpei215 Dec 31, 2017

Member

I think you could revert timestamp() and change _updateField() method like the following:

protected function _updateField($entity, $field, $refreshTimestamp)
{
    if ($entity->isDirty($field)) {
        return;
    }

    $ts = $this->timestamp(null, $refreshTimestamp);

    $columnType = $this->getTable()->getScheme()->getColumnType($field);
    $type = Type::build($columnType);
    $class = $type->getDateTimeClassName();

    // Create a new proper DateTimeInterface instance here

    $entity->set($field, $ts);
}

This comment has been minimized.

@o0h

o0h Dec 31, 2017

Contributor

thanks, i'll do it!

@o0h

This comment has been minimized.

Contributor

o0h commented Dec 30, 2017

thanks.
can i change this pr based-branch master to 3.next ?

@dereuromark dereuromark changed the base branch from master to 3.next Dec 30, 2017

@dereuromark

This comment has been minimized.

Member

dereuromark commented Dec 30, 2017

I adjusted the PR base. But you might want to rebase on top of latest commit to resolve the conflicts.

@dereuromark dereuromark added this to the 3.6.0 milestone Dec 30, 2017

@dereuromark

This comment has been minimized.

Member

dereuromark commented Dec 30, 2017

You can also create a patch from your local diff to master and re-apply that patch to a fresh 3.x branch and then force push it to this one here. That might be quickest.

@o0h

This comment has been minimized.

Contributor

o0h commented Dec 30, 2017

thanks. i pushed force my branch

@chinpei215

This comment has been minimized.

Member

chinpei215 commented Dec 31, 2017

You can ignore EventManagerTest errors, as I have already fixed it in my pull request.

];
$table = new Table(['schema' => $schema]);
return $table;

This comment has been minimized.

@stickler-ci

stickler-ci Dec 31, 2017

Contributor

Language constructs must be followed by a single space; expected 1 space but found 2
Double space found

@o0h

This comment has been minimized.

Contributor

o0h commented Dec 31, 2017

implemented in _updateField(), and added an test.

@markstory markstory merged commit 2d21e3a into cakephp:3.next Jan 2, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
stickler-ci No lint errors found.

markstory added a commit to cakephp/docs that referenced this pull request Jan 2, 2018

@o0h o0h deleted the o0h:timestamp-behavior-follow-datetimetype-set-immutable branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment