Hi,
I made some improvements that would like to get merged into PHP, but I
require someone with Zend karma to review and merge.
The patches are all related and included in each other as a progressive
effort.
As a quick explanation, the removal of ZEND_ACC_FINAL_CLASS addresses some
classes that were not properly assigned as final. They're now final.
Examples are COM, PDO Statement, DOM XML and MySQLi.
They also reduce the amount of checks of bison when parsing a php file, it
provides a nicer fatal error around multiple final and multiple abstract
mix between abstract and final instead of a generic parser error.
Finally, ZEND_ACC_TRAIT got updated to a saner value. It was pretty complex
to comprehend why it was 0x120. It was done because a trait was considered
internally as abstract because of instantiation and inheritance checking.
This all got addressed too.
Here they are:
- ZEND_ACC_TRAIT value update: https://github.com/php/php-src/pull/931
(includes the next one) - Zend language parser class decoupling:
https://github.com/php/php-src/pull/928 (includes the next one) - ZEND_ACC_FINAL_CLASS removal: https://github.com/php/php-src/pull/911
Theoretically, merging the first one would mean merging all, but I left
them separate for proper case by case evaluation.
Regards,
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
The parser changes need to be careful reviewed; I don't have time at
the moment to verify it but I think you unintentionally allowed some
syntax's that shouldn't be valid because of the addition to
inner_statement.
Maybe I just looked too quickly. In any case, parser changes should
always get several people reviewing them.
The parser changes need to be careful reviewed; I don't have time at
the moment to verify it but I think you unintentionally allowed some
syntax's that shouldn't be valid because of the addition to
inner_statement.
Shouldn't. I broke down class_declaration_statement into 3 pieces:
class_declaration_statement, interface_declaration_statement and
trait_declaration_statement.
At the end, all I've done is adding the other 2 new rules back to where it
was consumed.
Maybe I just looked too quickly. In any case, parser changes should
always get several people reviewing them.
Agreed. =)
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
Hi guys,
I'd really appreciate some review around the before-mentioned PRs. I have
added a new one to the list now:
This PR addresses the parsing support for traits to have "extends" and
"implements", as they are invalid.
There's another one in the oven, which prevents extension developers to
create classes that extends traits or interfaces.
This is currently supported only for userland classes, but not for Zend API.
Cheers,
On Wed, Dec 3, 2014 at 8:39 PM, guilhermeblanco@gmail.com <
guilhermeblanco@gmail.com> wrote:
The parser changes need to be careful reviewed; I don't have time at
the moment to verify it but I think you unintentionally allowed some
syntax's that shouldn't be valid because of the addition to
inner_statement.Shouldn't. I broke down class_declaration_statement into 3 pieces:
class_declaration_statement, interface_declaration_statement and
trait_declaration_statement.
At the end, all I've done is adding the other 2 new rules back to where it
was consumed.Maybe I just looked too quickly. In any case, parser changes should
always get several people reviewing them.Agreed. =)
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
I don't see technical problems with the patch.
However, I also don't see any significant benefits.
From the user perspective it'll just change error messages and prevent
"final final class" declarations.
Nikita, what do you think?
Thanks. Dmitry.
On Fri, Dec 5, 2014 at 8:18 PM, guilhermeblanco@gmail.com <
guilhermeblanco@gmail.com> wrote:
Hi guys,
I'd really appreciate some review around the before-mentioned PRs. I have
added a new one to the list now:This PR addresses the parsing support for traits to have "extends" and
"implements", as they are invalid.There's another one in the oven, which prevents extension developers to
create classes that extends traits or interfaces.
This is currently supported only for userland classes, but not for Zend
API.Cheers,
On Wed, Dec 3, 2014 at 8:39 PM, guilhermeblanco@gmail.com <
guilhermeblanco@gmail.com> wrote:The parser changes need to be careful reviewed; I don't have time at
the moment to verify it but I think you unintentionally allowed some
syntax's that shouldn't be valid because of the addition to
inner_statement.Shouldn't. I broke down class_declaration_statement into 3 pieces:
class_declaration_statement, interface_declaration_statement and
trait_declaration_statement.
At the end, all I've done is adding the other 2 new rules back to where
it
was consumed.Maybe I just looked too quickly. In any case, parser changes should
always get several people reviewing them.Agreed. =)
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
I don't see technical problems with the patch.
However, I also don't see any significant benefits.
From the user perspective it'll just change error messages and prevent
"final final class" declarations.Nikita, what do you think?
Thanks. Dmitry.
The three parts:
-
Use ZEND_ACC_FINAL instead of ZEND_ACC_FINAL_CLASS: Looks sensible,
given how many extensions have confused this. We should be careful that
this change does not make anything final that many people extended (even if
it was originally meant to be final.) -
Don't use magic value for ZEND_ACC_TRAIT: Also makes sense, the behavior
of the current value is pretty unclear. -
Changing the grammar to separate class/trait declarations and have a
modifier list: This doesn't really make much sense as things currently are
(only one modifier allowed). I'd suggest to change this when we actually
support multiple modifiers (e.g. with the static classes patch).
Nikita
Nikita,
Shouldn't #3 make more sense taking into consideration this commit:
https://github.com/guilhermeblanco/php-src/commit/872a97c8dbc1c8823985d9a0305938c508865a0d
It is part of a followup PR https://github.com/php/php-src/pull/937 that
removes compiler code checks and delegates to bison, since it's a grammar
issue to accept "trait Foo extends Bar".
Regards,
I don't see technical problems with the patch.
However, I also don't see any significant benefits.
From the user perspective it'll just change error messages and prevent
"final final class" declarations.Nikita, what do you think?
Thanks. Dmitry.
The three parts:
Use ZEND_ACC_FINAL instead of ZEND_ACC_FINAL_CLASS: Looks sensible,
given how many extensions have confused this. We should be careful that
this change does not make anything final that many people extended (even if
it was originally meant to be final.)Don't use magic value for ZEND_ACC_TRAIT: Also makes sense, the
behavior of the current value is pretty unclear.Changing the grammar to separate class/trait declarations and have a
modifier list: This doesn't really make much sense as things currently are
(only one modifier allowed). I'd suggest to change this when we actually
support multiple modifiers (e.g. with the static classes patch).Nikita
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
On Thu, Dec 4, 2014 at 1:46 AM, guilhermeblanco@gmail.com
guilhermeblanco@gmail.com wrote:
Examples are COM, PDO Statement, DOM XML and MySQLi.
They also reduce the amount of checks of bison when parsing a php file, it
provides a nicer fatal error around multiple final and multiple abstract
mix between abstract and final instead of a generic parser error.
PDO statement should not be made final, or this is a breaking language
change (requiring an RFC and a 2/3 majority).
Drupal 7 does extend PDOStatement without any problem.
Damien
PDO statement should not be made final, or this is a breaking language
change (requiring an RFC and a 2/3 majority).
I see from the PR that it's PDORow, not PDOStatement. That one is not
actually exposed as such to userspace, so it is fine.
Damien
Hi Dmitry,
As I said, these patches are not huge beneficial, but enablers. Enablers in
the sense that further development over class/interface/trait simplified.
It's not a short win benefit, but a mid/long term.
Also defer currently checks done in zend_compile to Bison (such as trait
extends and implements), addresses small inconsistencies (such as an
extension class that extend an interface) and also easier to comprehend for
newcomers (such as trait flag value).
Now development that happens over this (such as abstract final/static
class, package private class) become easy, as you can see in PR for static
class for example.
Cheers,
PDO statement should not be made final, or this is a breaking language
change (requiring an RFC and a 2/3 majority).I see from the PR that it's PDORow, not PDOStatement. That one is not
actually exposed as such to userspace, so it is fine.Damien
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada
I'm not against the patch, I'm actually indifferent.
Lets wait for Nikita's opinion.
I took just a quick look over "static class" RFC.
I'll need to think more about it.
Thanks. Dmitry.
On Mon, Dec 8, 2014 at 5:18 PM, guilhermeblanco@gmail.com <
guilhermeblanco@gmail.com> wrote:
Hi Dmitry,
As I said, these patches are not huge beneficial, but enablers. Enablers in
the sense that further development over class/interface/trait simplified.
It's not a short win benefit, but a mid/long term.Also defer currently checks done in zend_compile to Bison (such as trait
extends and implements), addresses small inconsistencies (such as an
extension class that extend an interface) and also easier to comprehend for
newcomers (such as trait flag value).Now development that happens over this (such as abstract final/static
class, package private class) become easy, as you can see in PR for static
class for example.Cheers,
PDO statement should not be made final, or this is a breaking language
change (requiring an RFC and a 2/3 majority).I see from the PR that it's PDORow, not PDOStatement. That one is not
actually exposed as such to userspace, so it is fine.Damien
--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada