cphalcon icon indicating copy to clipboard operation
cphalcon copied to clipboard

[BUG]: Saving related entity in Phalcon 5

Open slechtic opened this issue 3 years ago • 19 comments

Discussed in https://github.com/phalcon/cphalcon/discussions/16203

Originally posted by slechtic November 11, 2022 Hi,

I'm trying to migrate from Phalcon v3 to v5 but I have a problem with updating related entity.

I have an entity Document and this entity has related entity DocumentHistory.

On Document entity there is hasMany relationship defined:

$this->hasMany('id', DocumentHistory::class, 'idDocument', ['alias' => 'documentHistories']);

DocumentHistory entity has belongsTo relationship:

$this->belongsTo('idDocument', Document::class, 'id',['alias' => 'document']);

when I create new DocumentHistory entity and assign existing Document entity, change some values on this entity (Document) and save DocumentHistory entity, only DocumentHistory is saved (created). Document entity is not updated but foreign key is set on DocumentHistory entity correctly. In case when both entities exist it works correctly.

$documentHistory = new DocumentHistory();

$document = Document::findFirst($id);
$document->setNumber($number);

$documentHistory->setDocument($document);
$documentHistory->save();

On DocumentHistory there is a setter:

public function setDocument(Document $document) {
    $this->document = $document;
}

This works for Phalcon v3. This will not be supported in v5 or it is a bug? Thank you.

My environent:

PHP 8.1. Phalcon 5.1.0. Postgresql 11

slechtic avatar Nov 22 '22 07:11 slechtic

Hi guys, did you have any chance to check this problem? We need to know whether it is a bug or expected behavior so that we can proceed with Phalcon 5 upgrade.

slechtic avatar Dec 09 '22 09:12 slechtic

Did you tried to migrate to v4 first?

Jeckerson avatar Dec 09 '22 17:12 Jeckerson

Did you tried to migrate to v4 first?

Yes I did. But there was a problem with segmentation fault and suggestion was migrate to v5. https://github.com/phalcon/cphalcon/discussions/16028

slechtic avatar Dec 09 '22 17:12 slechtic

Here you can test it https://github.com/slechtic/phalcon5_relationships

slechtic avatar Dec 16 '22 09:12 slechtic

Hi @Jeckerson could you please let us know whether this is expected behavior, bug, or we are using it wrong?

In the previous post from @slechtic, there is a docker image with the example of the issue where you can easily test it.

Is there someone from Phalcon team who can help us even as a paid consultation on this matter?

Thank you in advance!

Marek

markofo avatar Jan 05 '23 14:01 markofo

Hi @Jeckerson, any progress? Did you try to run my example project which I attached?

slechtic avatar Jan 26 '23 11:01 slechtic

Hi @Jeckerson,

I have encountered the same issue as the OP. Could you give us an information whether it is a bug or not? I need to decide if I need to migrate my project to another FW.

GeoffreyCZ avatar Feb 21 '23 11:02 GeoffreyCZ

Hi, I've run into the same issue while migrating a project to newer Phalcon.

Šlechtič's docker image (from the link above) clearly illustrates the issue I have.

Can you please check it and tell us what went wrong? Thanks in advance

MartinZubek avatar May 10 '23 08:05 MartinZubek

Hi @Jeckerson, same here. We are stuck with the upgrade of our current version to v5.

FPEPOSHI avatar May 15 '23 11:05 FPEPOSHI

I'll investigate.

Jeckerson avatar May 15 '23 13:05 Jeckerson

Hi @Jeckerson, do we have any update about the issue reported?

FPEPOSHI avatar Jun 06 '23 14:06 FPEPOSHI

I'm not sure this is related to this bug, but this doesn't work either:

public function initialize() {
  $this->hasOne('code', 'ApiCore\App\Models\VendorPricingConfig', 'vendor_code', ['alias' => 'pricingConfig']);
}

public static function suspendVendor($vendorCode, DateTime $toDate) {
  $model = self::findFirst(array(
     'conditions' => 'code = ?1',
     'bind' => array(1 => $vendorCode)
  ));
  
  $model->pricingConfig->auto_disabled = 1;
  $model->pricingConfig->auto_disabled_date = $toDate->format('Y-m-d H:i:s');
  $model->save();
}

This only sets auto_disabled on $model->pricingConfig to 1 and doesn't set auto_disabled_date. If I invert those two lines

$model->pricingConfig->auto_disabled_date = $toDate->format('Y-m-d H:i:s');
$model->pricingConfig->auto_disabled = 1;

..it only sets auto_disabled_date.

https://github.com/phalcon/cphalcon/pull/16402 doesn't fix it

maxgalbu avatar Sep 04 '23 16:09 maxgalbu

@maxgalbu there are two problems, 1 - the getRelated fetchs always a new record so everytime you do $model->pricingConfig, you effectively call a new model.

2 - lack of a First Level Cache Hopefully I can add it to phalcon5.

Anyway a way to solve this:

$pricingConfig = $model->pricingConfig;
$pricingConfig->auto_disabled = 1;
$pricingConfig->auto_disabled_date = $toDate->format('Y-m-d H:i:s');

$model->pricingConfig = $pricingConfig;
$model->save();

The $model->pricingConfig = $pricingConfig; could be $model->pricingConfig = $model->pricingConfig;

This would put the pricingConfig in the dirtyRelated array, that is the first one to be fetched. And it's the best way to mark a dirtyRelation, so it's probably going to be a default behaviour.

Hope this helps.

rudiservo avatar Sep 21 '23 16:09 rudiservo

@rudiservo yes, I solved it like you said. Still, it's weird? that there's no cache on the related models

maxgalbu avatar Sep 27 '23 14:09 maxgalbu

@maxgalbu yeah, trying to fix that, will make a few PR to see how it goes, but still to make stuff dirty I do recommend the $model->pricingConfig = $model->pricingConfig to set it has dirtyRelated, will make it easier in the save specially if I can add teh option for collectRelatedToSave to only return the dirtyRelated instead of all relations.

Hopefully in the near future.

rudiservo avatar Sep 27 '23 14:09 rudiservo

@rudiservo Hi, do you already have any plan to fix and release the issue with saving the related entity? Thank you!

markofo avatar Nov 28 '23 16:11 markofo

@markofo Plans, yes, actual code yes, but still ironing out some kinks, release I am not sure, it adds some behaviour, although some is optional, it's still pretty new. The focus is v6 for the time, we do not want to add to many things to v5 ATM. I do have a repo with every idea I have but it's not stable enough, I need to test it for 2 more weeks. PR #16402 will need a small function and it's stable. Anyway, has soon has I get my branches stable and v6 is out, I will try and put out a release for v5, although with v6 might be a better option to work with IMO. Sorry.

rudiservo avatar Nov 28 '23 17:11 rudiservo

@rudiservo it is not a new thing, this worked in Phalcon 3, that's why we were wondering about getting this fixed in 5. We have to workaround this issue for the time being.

Regarding Phalcon 6 that you mentioned, when do you expect the 6 to be released, anytime soon?

@markofo Plans, yes, actual code yes, but still ironing out some kinks, release I am not sure, it adds some behaviour, although some is optional, it's still pretty new. The focus is v6 for the time, we do not want to add to many things to v5 ATM. I do have a repo with every idea I have but it's not stable enough, I need to test it for 2 more weeks. PR #16402 will need a small function and it's stable. Anyway, has soon has I get my branches stable and v6 is out, I will try and put out a release for v5, although with v6 might be a better option to work with IMO. Sorry.

markofo avatar Jan 17 '24 13:01 markofo

@markofo it worked on phalcon3 but it had it's own set of issues that needed a workaround.

No ETA on phalcon6, I am swamped at work ATM.

rudiservo avatar Jan 17 '24 13:01 rudiservo