fix(array): cast numeric string keys into zend_ulong and allow negative keys

[kakserpom]

#453

[Xenira]

@kakserpom thank you so much!

Didn't have a detailed look yet, but remembered that we have an ArrayKey enum. Might want to utilise it here.

[kakserpom]

PR solves both #453 and #454.

[Xenira]

@kakserpom Would you mind adding integration and unit tests?

[coveralls]

Pull Request Test Coverage Report for Build 16000972716

Details

• 0 of 81 (0.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 20.197%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/zend/handlers.rs	0	1	0.0%
src/types/array.rs	0	80	0.0%

Totals
Change from base Build 15999426440:	-0.3%
Covered Lines:	760
Relevant Lines:	3763

---

💛 - Coveralls

[Xenira]

@kakserpom just a hint for commit messages you can use convco

[kakserpom]

@Xenira Ordnung über alles? 😁

[Xenira]

@kakserpom keep it in english pls 😉

[Qard]

Looks like this needs a rebase and to get rid of the clippy bits now that the clippy updated PR merged. Other than that though, LGTM. 🙂

[kakserpom]

@Qard done

[Qard]

What about all the #[allow(clippy::mut_from_ref)] lines? Are those actually necessary?

[kakserpom]

@Qard yes, otherwise Clippy shows warnings because of &self instead of &mut self in function signatures.

[Qard]

Ah, so it's intentional to leave it then? Just wanted to be sure we weren't just forgetting to un-silence clippy in some places it might have legitimate warnings. 😅

[kakserpom]

Yes, It's intentional. I've enabled clippy::pedantic in another PR and quite a few warnings appeared, some of them led to actual fixes and some had to be silenced.

[Qard]

The clippy changes I guess aren't related to this PR, but necessary to get CI to pass, so that's fine.

Everything LGTM. Hopefully this can land soon to unblock other things. 🙂

[Xenira]

@kakserpom @Qard Regarding the clippy issues: Our usual procedure is to create a mr fixing those issues and rebase affected MRs. I have created one for the current issues, except the ones in #461, as those require some more investigation and possible breaking changes.

@kakserpom could you remove those ignores and rebase once #474 is merged?

[Xenira]

Only some small changes needed.

Could you also add integration tests for negative and 'string number' indices?

[kakserpom]

@Xenira

Could you also add integration tests for negative and 'string number' indices?

I'd rather someone else do it because I am on a trip until the end of the week. It's pretty trivial.

[Xenira]

I'd rather someone do it because I am on a trip until the end of the week. It's pretty trivial.

Sure, will add them :)