Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List refactoring (v6) #8773

Closed
wants to merge 114 commits into from
Closed

Conversation

danog
Copy link
Collaborator

@danog danog commented Nov 26, 2022

Fixes a bunch of bugs all over the codebase.

@danog danog marked this pull request as ready for review December 2, 2022 20:43
@danog
Copy link
Collaborator Author

danog commented Dec 2, 2022

Wew, this PR triggered a huge amount of previously uncovered and broken edge cases, I fixed a bunch of bugs also present in master.
I've been thinking of merging this into master for v6, and then backporting a TList-compatible version (that removes TList-specific logic but keeps compatibility by converting plugin-originated TLists into the equivalent TKeyedArrays before handling, possibly directly inside the constructor of Union).

@danog
Copy link
Collaborator Author

danog commented Dec 2, 2022

The HasArrayKey assertion turned out to be completely broken and actually useless, for example when asserting array_key_exists($key, $arr) it asserted the existance of $arr['$key'] (not $arr[$key]), only to bypass a bug in class constant key assertions (which I also fixed in this PR).
It might be worth completely removing it.

Edit: nevermind it's only really used properly for class strings (still a bit weird though), reverted that part.

@danog danog changed the title List refactoring List refactoring (v6) Dec 3, 2022
@danog
Copy link
Collaborator Author

danog commented Dec 3, 2022

Closing until the master branch becomes the v6, refer to #8820

@danog danog closed this Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants