search

Kyslik / column-sortable

Package for handling column sorting in Laravel 5/6/7/8
MIT License
644 stars 105 forks source link

Update SortableLink.php #145

Open seche opened 4 years ago

seche commented 4 years ago

Fixes #144 Updated Query String to override properly the given Get variables rather than ignoring them.

Kyslik commented 3 years ago

I fear that this will fix the issue you have but it may break the package use for others 😢 .

robbielove commented 2 years ago

you could add a config setting @seche - this way we can avoid this behaviour by default (users who don't have the updated config - null) and then if true you can swap the order of the params

it would be nice if you could actually document or explain in more detail what this PR does and why its needed, or why someone would want to swap the params

seche commented 2 years ago

@Kyslik How can it break anything? Its just setting the order of which a value is set. And like I described in #144 as it stands, @sortablelink() it ignores all values that you give it if they are already set. So anyone who would have set a value that exist would be in the same boat as myself where it doesn't work, and otherwise the values get merged as it is already.

@robbielove All the details are in #144 I could set a config setting but this seems more of a error fix than a feature. But in the end it's the only way to have multiple sortable tables with links on the same page.

From PHP DOC: Array Merge ... If the input arrays have the same string keys, then the later value for that key will overwrite the previous one. ...

robbielove commented 2 years ago

This PR has been open for 2 years and others are also starting to need this functionality.

I would suggest that we merge my PR (#194 ) which preserves the existing functionality; which should satisfy the fears of @Kyslik and allows the functionality that @seche and others such as myself and those on #200 desire by changing a simple config value.

Can we all agree that this is a good enough compromise to unblock these PR's?