Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 15, 2025

No description provided.

@Girgias Girgias force-pushed the pgsql-abstract-class branch 3 times, most recently from 4db2580 to dc642e8 Compare October 15, 2025 14:25
@Girgias Girgias marked this pull request as ready for review October 15, 2025 14:25
@Girgias Girgias requested a review from devnexen as a code owner October 15, 2025 14:25
@Girgias
Copy link
Member Author

Girgias commented Oct 15, 2025

@devnexen do you know why Windows fails, it says there is an issue with the CLEAN section of the test

@devnexen
Copy link
Member

I have no real idea honestly.

} catch(Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would it be possible to add the closing tag ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also be the reason windows is confused

$result = pg_query($db, $sql);
var_dump(pg_fetch_object($result, NULL, 'E'));
} catch(Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try without this last PHP_EOL wdyt ?

@Girgias Girgias force-pushed the pgsql-abstract-class branch from dc642e8 to c48266c Compare October 21, 2025 20:03
@Girgias Girgias requested a review from nielsdos October 21, 2025 20:10
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test this myself, but the code changes look good.
I don't think you need all those CE variants (i.e. interface, enum, ...) for the test, but it doesn't hurt.

@Girgias
Copy link
Member Author

Girgias commented Oct 21, 2025

I don't think you need all those CE variants (i.e. interface, enum, ...) for the test, but it doesn't hurt.

I mainly wanted them to check the error messages, but they indeed can't hurt (especially considering how poorly tested ext/pgsql is)

This shouldn't really be happening in the first place
include('config.inc');
$db = pg_connect($conn_str);
pg_query($db, "DROP TABLE IF EXISTS pg_fetch_object_abstract_class cascade");
$db = @pg_connect($conn_str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue really weirds me out. would it be possible to do the cleaning somewhere in the file section instead just for testing once at least ?

@nielsdos nielsdos requested a review from TimWolla as a code owner October 22, 2025 21:48
@nielsdos
Copy link
Member

So it fails because the CLEAN section somehow outputs " \r\n" and that's not equal to the empty string so it borks. I don't know what produces these whitespace characters. Pushing some more debugging code...

@nielsdos
Copy link
Member

Will continue debugging this later ig. The simple fix is just get rid of the clean section and do the table deletion in the FILE section, that's how the other tests do it.
I don't know what causes the extra whitespace output.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants