Skip to content

Update SortableLink.php - #145

Open
seche wants to merge 1 commit into
Kyslik:L6from
seche:L6
Open

Update SortableLink.php#145
seche wants to merge 1 commit into
Kyslik:L6from
seche:L6

Conversation

@seche

@seche seche commented Feb 24, 2020

Copy link
Copy Markdown

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

Updated Query String to override properly the given Get variables rather than ignoring them.
@Kyslik

Kyslik commented Mar 25, 2021

Copy link
Copy Markdown
Owner

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

@robbielove

Copy link
Copy Markdown

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

@robbielove robbielove mentioned this pull request Feb 9, 2022
@seche

seche commented May 30, 2022

Copy link
Copy Markdown
Author

@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

Copy link
Copy Markdown

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?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@sortablelink parameter #3 not updating and using current value

3 participants