m6w6 / ext-pq

PostgreSQL client library (libpq) binding
BSD 2-Clause "Simplified" License
39 stars 7 forks source link

Mark properties as read-only when no write method #50

Closed remicollet closed 1 year ago

remicollet commented 1 year ago

Supported by PHP 8.1+ no change for older versions

codecov[bot] commented 1 year ago

Codecov Report

Merging #50 (eea6e05) into master (fbbe06a) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #50   +/-   ##
=======================================
  Coverage   76.82%   76.82%           
=======================================
  Files          17       17           
  Lines        3612     3612           
=======================================
  Hits         2775     2775           
  Misses        837      837           
Impacted Files Coverage Δ
src/php_pqcancel.c 89.39% <100.00%> (ø)
src/php_pqconn.c 67.77% <100.00%> (ø)
src/php_pqcopy.c 85.98% <100.00%> (ø)
src/php_pqcur.c 69.84% <100.00%> (ø)
src/php_pqlob.c 85.92% <100.00%> (ø)
src/php_pqres.c 84.45% <100.00%> (ø)
src/php_pqstm.c 88.68% <100.00%> (ø)
src/php_pqtxn.c 54.40% <100.00%> (ø)
src/php_pqtypes.c 90.27% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

m6w6 commented 1 year ago

Hmm, after a quick thought, I'm skeptical that a lot of those properties actually adhere to PHP's readonly semantics, but are rather private::set, which doesn't exist in PHP...

m6w6 commented 1 year ago

Meaning: lots of them, may change the value during lifetime...

remicollet commented 1 year ago

Please check for "socket" property which use std handler, so can probably be changed...

remicollet commented 1 year ago

Meaning: lots of them, may change the value during lifetime...

Hmmm I understand read-only, as cannot be set by user, not as immutable...

m6w6 commented 1 year ago

Yeah, that's what extension authors usually "declared" readonly, but if ZEND_ACC_READONLY resembles userland readonly, it means something totally different, doesn't it?

A readonly property can only be initialized once, and only from the scope where it has been declared. Any other assignment or modification of the property will result in an Error exception.

remicollet commented 1 year ago

Following code is very confusing

$con = new pq\Connection(PQ_DSN);
var_dump($con->status);
var_dump($con->status = 125);
var_dump($con->status);

Output:

int(0)
int(125)
int(0)

And strangely ZEND_ACC_READONLY change nothing (we may expect some error/exception)

remicollet commented 1 year ago

Yeah, that's what extension authors usually "declared" readonly, but if ZEND_ACC_READONLY resembles userland readonly, it means something totally different, doesn't it?

A readonly property can only be initialized once, and only from the scope where it has been declared. Any other assignment or modification of the property will result in an Error exception.

https://wiki.php.net/rfc/readonly_properties_v2 also says

However, readonly properties do not preclude interior mutability. Objects (or resources) stored in readonly properties may still be modified internally:

remicollet commented 1 year ago

Understood, readonly only apply to typed property, so have to

-       zend_declare_property_long(php_pqconn_class_entry, ZEND_STRL("status"), CONNECTION_BAD, ZEND_ACC_PUBLIC|ZEND_ACC_READONLY);
+       zval status_default_value;
+       ZVAL_UNDEF(&status_default_value);
+       zend_string *status_column_name = zend_string_init("status", sizeof("status") - 1, 1);
+       zend_declare_typed_property(php_pqconn_class_entry, status_column_name, &status_default_value, ZEND_ACC_PUBLIC|ZEND_ACC_READONLY, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG));

And then

Fatal error: Uncaught Error: Cannot initialize readonly property pq\Connection::$status from global scope in /work/GIT/pecl-and-ext/pq/prop.php:12

Else the readonly attribute is only for documentation (reflection)

m6w6 commented 1 year ago

Objects (or resources) stored in readonly properties may still be modified internally

^^^^^ Read that again ;-) objects and resources (i.e. objects containing other properties, or e.g. file contents of underlying resources)

remicollet commented 1 year ago

OK, so we don't have same idea ;)