Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

Laravel 4.2 soft delete doesn't work with Ardent #211

Closed
IntellectProductions opened this issue Jun 6, 2014 · 25 comments
Closed

Laravel 4.2 soft delete doesn't work with Ardent #211

IntellectProductions opened this issue Jun 6, 2014 · 25 comments

Comments

@IntellectProductions
Copy link

Ok so Laravel 4.1 with the protected $softDelete = true works fine with Ardent. But when you upgrade to Laravel 4.2, it doesn't work anymore since they changed the way soft delete is set. It will still remove the record, but it will remove it permanently instead of updating the deleted_at column.. Do you know when there will be a fix for this?

http://laravel.com/docs/upgrade#upgrade-4.2

Thanks!

@CasperLaiTW
Copy link

Hi, I got same problem and I fix.
You can try #212 patch.

@divdax
Copy link

divdax commented Jun 10, 2014

Please merge! 😄

@IntellectProductions
Copy link
Author

Awesome thank you ! :D

mspivak added a commit to mspivak/ardent that referenced this issue Jul 16, 2014
Ardent->morphTo() was overriding a new eager loading check on Laravel 4.2.
mspivak added a commit to mspivak/ardent that referenced this issue Jul 16, 2014
`Ardent->morphTo()` was overriding the check for eager loading on new Laravel 4.2 Eloquent Model.
laravel-ardent#211
@mspivak
Copy link

mspivak commented Jul 16, 2014

This is actually not fixing it. Check this one out: #227

mspivak added a commit to mspivak/ardent that referenced this issue Jul 16, 2014
@chriskonnertz
Copy link
Contributor

Is there actually someone still maintaining Ardent? Doesn't look like someone is working on a fix or at least a workaround for this problem.

@wizardist
Copy link

Is there actually someone still maintaining Ardent? Doesn't look like someone is working on a fix or at least a workaround for this problem.

As you can see, the latest commit was submitted on Jul 10. Could anyone reach the maintainers?

@jloosli
Copy link

jloosli commented Aug 29, 2014

On a related note, I was able to get the soft delete to work by using

protected $softDeletes = true

in addition to

use SoftDeletingTrait;
protected $dates = ['deleted_at'];

However, calling

$user = User::withTrashed()->where( 'id', $id )->first();
// At this point, I've verified that a user is found
$user->restore();
// Should be restored at this point
$user = User::find($id);
// However $user is null after searching for this $id.

I've also tried

$user->deleted_at = null;
$user->save();

and that doesn't seem to work either.

@divdax
Copy link

divdax commented Aug 30, 2014

For me Ardent is dead. It's not really be maintained any more...
Now i'm using https://github.com/dwightwatson/validating

@chriskonnertz
Copy link
Contributor

@jloosli Yep, I also tried the "protected $softDeletes = true" trick but it's not solving all problems...

@ananddpatil
Copy link

@jloosli Any updates on the restore() issue?

@chriskonnertz
Copy link
Contributor

I've dropped Ardent from my app, because I assume it's no longer maintained. I've copied all methods that provide support for "cleaner relationships" into my base model so that's still working. For validation I'll use dwightwatson/validating.

@jloosli
Copy link

jloosli commented Sep 16, 2014

@ananddpatil No. I haven't had a chance to do it yet, but I'll probably try my luck at dwightwatson /validating as well.

@dandonahoe
Copy link

For the love of god someone please fix this. I just spent 4 hours tracking down this bug.

@chriskonnertz
Copy link
Contributor

Nah, it's not going to happen. Ardent is dead. Laravel 5 is incoming and Ardent isn't even working with 4.2... Just forget about it.

@igorsantos07
Copy link
Member

This is being looked at. If there's still someone here, please lend me a hand at #227.
The original maintainer abandoned the project almost completely, and I tried to hold it, but I don't have enough time to look at it, besides health issues. I'm trying to catch up, as the package still have a lot of daily downloads.

@chriskonnertz
Copy link
Contributor

I don't know if mspivak@19d231e solves the problem or not. I do use my own fork of Ardent. Therefore I cannot test the change. But it looks promising.

PS: It's very kind that you maintain this package so those who still rely on it get some support.

@thinksaydo
Copy link

thanks @chriskonnertz. you are correct. @mspivak commit fixed this issue for us for an older Laravel 4.2 project. his other commit also fixed the morphTo issue. cc @igorsantos07

@igorsantos07
Copy link
Member

Thanks for the followup, @chriskonnertz and @thinksaydo. Would you please help me review the PR #227? Specifically this part here. That seems quite odd to me.
With that solved I'll merge and create a new release - that's already in draft, just waiting for those bigger issues to be solved.

@chriskonnertz
Copy link
Contributor

Honestly I am very disappointed by Ardent. I had to fix the bug myself and I had to create a very dirty fork. The only way that I could help you is to merge the changes of the PR into my fork and I am not going to do that. I do not want to risk that. I do not trust Ardent any more and therefore I do not plan to use it again.

I am sure you are doing a great job but I needed that bugfix in August 2014... Plus, I do not use morphTo thus I am not the right person to judge about that PR.

PS: My fork does not even override Eloquent's morphTo method.

@chriskonnertz
Copy link
Contributor

Take a look at this very old version: https://github.com/mspivak/ardent/blob/9fbe73399d84fc726dc9e122955de444f4fb4901/src/LaravelBook/Ardent/Ardent.php In line 428 there it is using belongTo. So I guess the red lines in mspivak@1228dfe are just something left over from July 2014. It should not change anything if you merge it.

@chriskonnertz
Copy link
Contributor

I believe you can accept the PR. Or refuse it and create a new PR by yourself. Because essentially all it does is to update the newQuery() method so that it does not use old soft deleting stuff any more.

I have created a new PR without the strange part ( #227 ) of the old one: #269

PS: Not tested!!!

@igorsantos07
Copy link
Member

I'm going to accept your new PR, @chriskonnertz.
I'm sorry for your disappointment, but I'm doing my best at this side. I can feel your pain as I was feeling the same when I decided to work on the repository myself... Sadly, the owner felt of the face of the earth and I got no control over some parts of the project - like the Github Page or Composer package.
I'm trying to catch up. If you still are dependant on Ardent - or your own fork - and are willing to help, I'll be glad to see you here again. You can PR new things you've done if you feel OSS-y :)

@igorsantos07
Copy link
Member

I've merged your PR directly as, per other discussions, that seems to be the fix.
Investigating a little bit more it seems that weird part was actually a fix for another creepy bug that wasn't noticed - and @mspivak fixed it without citing explicitly. Gonna open a new issue to investigate it.

@chriskonnertz
Copy link
Contributor

Ok.

@MrNomNom
Copy link
Contributor

damn. This PR broke my app... :) Looks like on 4.1 this change creates infinite loop of dependancy injections. Will investigate for more details...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet