fix issue 75: set and subobject fail execution when encountering invalid paramter type

SemanticMediaWiki / SemanticScribunto

Provides service functions to support the Scribunto extension

Other

23 stars 14 forks source link

fix issue 75: set and subobject fail execution when encountering invalid paramter type #76

Closed oetterer closed 4 years ago

oetterer commented 4 years ago

add parameter validation to mw.smw.lua
add integration tests for said fixes
fix php warning encountered in ScribuntoLuaEngineTestBase

This PR is made in reference to: #75

This PR addresses or contains: fixes for said issue

This PR includes:

[x] Tests (unit/integration)
[x] CI build passed

Fixes #75

oetterer commented 4 years ago

well, last test is still failing. can't see why, though...

oetterer commented 4 years ago

@kghbln unless someone can point me towards a fix for that last test, I suggest merging the PR

kghbln commented 4 years ago

@JeroenDeDauw Since PHP is not really my field of expertise I will be awesome if you could have a look why PHP 7.3 does not play here. I would like to help but I cannot.

oetterer commented 4 years ago

@kghbln found the problem, unfortunately fixing the last test breaks the first one. :(

kghbln commented 4 years ago

@oetterer

found the problem, unfortunately fixing the last test breaks the first one. :(

I am not sure what you mean. From my perspective still only PHP 7.3 gives pain and not PHP 7.0

If we have to decide between both versions support for PHP 7.0 could be dropped to make things easier.

oetterer commented 4 years ago

@kghbln Look at 92c4415. It contains the fix for the last test, but Travis fails in the first. Current #4c2b74a rolls that back.

kghbln commented 4 years ago

Ah, a circulum vitiosum. :( I would like to help but ...

oetterer commented 4 years ago

@kghbln just squash and merge?

kghbln commented 4 years ago

Wouldn't the test still fail?

oetterer commented 4 years ago

Yes. But it did so for quite some time now. Also, the reason why it fails has nothing to do with the patch. I merely tried to include a fix for it.

mwjames commented 4 years ago

Generally, thanks for your continued support and swift response to the feature request.

Yes. But it did so for quite some time now. Also, the reason why it fails has nothing to do with the patch. I merely tried to include a fix for it.

Let's be ruled by pragmatism here. This change will be part of a 2.2 release which allows you to kick PHP 7.0 support and means that you can apply your :void change.

On 11/16/19, Tobias Oetterer notifications@github.com wrote:

Yes. But it did so for quite some time now. Also, the reason why it fails has nothing to do with the patch. I merely tried to include a fix for it.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/SemanticMediaWiki/SemanticScribunto/pull/76#issuecomment-554422773

oetterer commented 4 years ago

And btw there is nothing broken functionality wise with mw master. A function declaration changed slightly and the test fails on that inconsistency.

kghbln commented 4 years ago

Cool, so the changes are void, er contain void? :)

oetterer commented 4 years ago

Cool, so the changes are void, er contain void? :)

The latter

kghbln commented 4 years ago

The latter

Didn't you revert?

oetterer commented 4 years ago

I reverted only the alleged fix for the failing test. The original patch is still there 9852730. So squash and merge and keep the comment from the first commit, pls. :)

kghbln commented 4 years ago

no wuckas