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

Fix soft deleted doesn't work when upgraded Laravel 4.2 #212

Closed
wants to merge 7 commits into from
Closed

Fix soft deleted doesn't work when upgraded Laravel 4.2 #212

wants to merge 7 commits into from

Conversation

CasperLaiTW
Copy link

Fix #211

@wemersonjanuario
Copy link

+1

@mtozlu
Copy link

mtozlu commented Jun 29, 2014

Thanks for the fix, it works like expected.

I would like to know if this causes any unknown side effects, since we are modifying query builder.
Thanks.

@GrahamCampbell
Copy link

Your indentation wrong.

@CasperLaiTW
Copy link
Author

I use four spaces replace with a tab. Is it wrong?

@GrahamCampbell
Copy link

You should indent with tabs only. No spaces.

@CasperLaiTW
Copy link
Author

Ok!!! I got. I will be fixed soon.
Thank you, @GrahamCampbell

@foozee
Copy link

foozee commented Jul 8, 2014

yes, that also fixes #220 to be able to utilize the new global scopes feature really hope to see this in master soon :)

@@ -865,14 +865,14 @@ public static function find($id, $columns = array('*')) {
return parent::find($id, $columns);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace! :(

@GrahamCampbell
Copy link

Can you fix the final issue, then squash this to 1 commit please.

@CasperLaiTW
Copy link
Author

:(
I fix.

@GrahamCampbell
Copy link

Still not squashed though. :/

@GrahamCampbell
Copy link

Can the whitespace for the dock block is wrong. There should be 1 tab, one space, then a star.

@CasperLaiTW
Copy link
Author

I miss the whitespace and fix!
Thanks you, @GrahamCampbell

@GrahamCampbell
Copy link

This really needs to be squashed.

@crhayes
Copy link

crhayes commented Jul 8, 2014

Is this going to get merged in soon?

@foozee
Copy link

foozee commented Jul 8, 2014

@igorsantos07 said that he's busy those days at #206 hopefully we can get this merged sooooon 👍

@igorsantos07
Copy link
Member

I'm slowly back to OS life. Sorry for this forever-wait :( I've been merging easy PRs for now.

@GrahamCampbell, actually tabs are not the correct indentation for this project. Take a look at #168 and read about PSR-1 and PSR-2, please :)
In other merges I've noticed our indentation was mixed up again 😔 I'll pull this by hand and fix the file myself, and speed up #168 so those issues can be tackled down more easily, or at least they'll get reduced.

As I'm not a Laravel expert I have the same doubt arrised by @1dot44mb. It looks like some important lines of code have been removed; why, and what would be the side effects on other usages?

@CasperLaiTW, I suggest you to read about squashing commits. A lot of small fixes like those would pollute the timeline. The best approach here would be creating a new branch with based on your master, squash things down, then create a new PR while closing this one - since doing a forced push to overwrite those commits is bad practice ®️ (although I tend to do it from time to time in small/personal projects haha). I can do the squashing by myself before merging, though, so this is more like a tip than a todo.

@CasperLaiTW
Copy link
Author

Welcome to back, @igorsantos07
Thanks your suggestion. I am reading about squashing commits.

I will rebase commits and create new PR.

This is good practice and I am imprinted on my mind.

@dalabarge
Copy link
Contributor

@igorsantos07 regarding the concerns of @1dot44mb, Laravel 4.2 introduced some traits and boot methods that handle global scopes such as softdeletes (which is now a trait). The Esensi/Model package includes soft deletes as a trait (among others including validation) with a ready to go model compatible with Laravel 4.2. You might glean some code insights from that package.

mspivak added a commit to mspivak/ardent that referenced this pull request Jul 16, 2014
@chrisatomix
Copy link

This has been causing problems for me when using Polymorphic relations, it was ignoring the SoftDeletingTrait and returning trashed rows in the MORPH_MANY results. As far as I can tell this problem is specific to Ardent, it doesn't affect Eloquent models.

In the end I worked around it by making some minor changes to my models.
I have a table called "notes", with a "parent_id" and "parent_type".
The Notes model contains the usual:

  public function parent() {
    return $this->morphTo();
  }

Inside the Ardent model for the parent (based on notes.parent_type) it uses this:

public function notes()
  {
    return $this->morphMany('Note','parent')->whereNull('deleted_at');
  }

This prevents trashed notes from being loaded.

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

Successfully merging this pull request may close these issues.

Laravel 4.2 soft delete doesn't work with Ardent
9 participants