API issue: Connection#$encoding

[DaveRandom]

Assigning this can fail. At the moment this can't be easily detected without installing global error handlers (it currently emits an E_NOTICE

ext-pq/src/php_pqconn.c

Line 199 in 75f3b1b

php_error(E_NOTICE, "Unrecognized encoding '%s'", zenc->val);

). Notably, it always fails if you attempt to set it before the connection has been established when connecting asynchronously.

As this is something that could contribute to a security issue if it fails, can we throw an exception instead? A failure to set an encoding anywhere should always be handled - or at least handle-able - IMHO.

[m6w6]

Implementing this as property was probably a bad idea, so, should we add a method and deprecate the public property?

[ParkFramework]

The ideal solution to implement lazy async connection :)

$db = new Connection($dsn); // new instance, not real connection to server

$db->encoding = 'UTF-8'; // if not or bad connect - save value, for deferred execute

yield from $db->exec($sql); // if not or bad connect - async connect and execute deferred query

I understand that it is very difficult, but it will be very convenient to use in userland.

[m6w6]

If the main concern is setting the encoding for an async connection, it's probably best to use the client_encoding connection string parameter: http://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNECT-CLIENT-ENCODING

[DaveRandom]

So I personally don't have any issues with a property assignment throwing. It's something other OO languages do all the time.

However, I will concede that it's not "the PHP way" and it would also be more of a BC issue than deprecating the property and replacing it with a pair of methods, so I'm happy to go with that. I'll check over the rest of the API for anything else that might present a similar problem.

[m6w6]

• Throw an exception on invalid property access
  (i.e. zend_replace_error_handling in php_pq_object_read_prop/php_pq_object_write_prop)
• Deprecate all connection sensitive properties and introduce getters/setters with exceptions
• Leave everything as it is now
  (i.e. E_NOTICE/E_WARNING/E_RECOVERABLE_ERROR (duh!))
• Did I miss something?