Please keep pull request discussions focused on implementation concerns. If you have general comments on an RFC, please make them on the PHP internals mailing list instead.

I apologize for offtopic, but is there a build in CI for Windows (https://windows.php.net/downloads/snaps/) with this PR or somewhere else? Or it makes sense to just wait for the merge?

// I can’t compile it at the moment

@SerafimArts latest CI build targeting windows is at https://ci.appveyor.com/project/php/php-src/builds/32298449

@Ocramius I meant binaries (artifacts)

Description

It seems that the validation of <<PhpAttribute>> doesn't work.

Test script

<?php
declare(strict_types=1);

class Test {
        public string $name;

        public function __construct(string $name) {
                $this->name = $name;
        }
}

<<Test('test')>>
function test(): void {}

$attrs = (new ReflectionFunction('test'))->getAttributes();

echo $attrs[0]->newInstance()->name . PHP_EOL;

Expected behavior

Fatal error: Attempting to use class 'Test' as attribute that does not have <<PhpAttribute>>.

Actual behavior

test

@moliata validation only happens when you call ->newInstance() on an attribute. This is specified in the RFC.

@moliata validation only happens when you call ->newInstance() on an attribute. This is specified in the RFC.

I'm not sure if I correctly understand you but I am using newInstance() in the test script.

@moliata ah sorry, weird that i didn't see that, i only saw the getAttributes line.

@beberlei after doing some tests, it seems I can trigger the check only if the class has some kind of an attribute. For example, let's reuse the same test script I used in my bug report and add something like this:

<?php
declare(strict_types=1);

// Adding any other than the <<PhpAttribute>> attribute to
// the class makes the check work and raises a fatal error
<<TestAttribute>>
class Test {
        public string $name;

        public function __construct(string $name) {
                $this->name = $name;
        }
}

<<Test('test')>>
function test(): void {}

$attrs = (new ReflectionFunction('test'))->getAttributes();

echo $attrs[0]->newInstance()->name . PHP_EOL;

EDIT: fixed a TYPO

@kooldev done.

@beberlei You need to adjust zend_execute.c and add another NULL, after arg_info (line 140) in the struct initializer. This is needed because you extended the zend_internal_function structure.

@nikic We are ready for final review (hopefully :))

The only thing that is not adressed from your previous comments is moving internal_validators global HashTable to compiler_globals.

Open todos are:

• Check scope resolution test, some funky things happening there with bindings, missing bindTo tests.
• ZVAL_COPY suggestion in php_reflection.c memory leaks
• attributes part of child array in zend_ast_decl
• IS_SERIALIZED / IS_UNSERIALIZED in file opcache
• Add a test for an attribute on an internal class/function

@beberlei All builds failed due to use of typed properties in zend_exceptions.c in lines 742 and 747. This has been committed yesterday. For some reason CI builds seem to use code from latest master..? You need to rebase attributes_v2_rfc on latest master and add a NULL, argument before use of (zend_type) in calls to zend_declare_typed_property() to fix compiler errors.

@kooldev You might want to not touch the zend_declare_property_ex and zend_declare_typed_property APIs at all, and instead add an additional lower-level zend_declare_attributed_property.

@nikic How about providing additional functions to do attribute registration and automatic HashTable initialization in a unified way? I implemented it that way in my attributes PR to avoid making changes to existing function signatures. I would like to add these function in zend_attributes.h:

ZEND_API zend_attribute *zend_add_class_attribute(zend_class_entry *ce, zend_string *name, uint32_t argc);
ZEND_API zend_attribute *zend_add_function_attribute(zend_function *func, zend_string *name, uint32_t argc);
ZEND_API zend_attribute *zend_add_parameter_attribute(zend_function *func, uint32_t offset, zend_string *name, uint32_t argc);
ZEND_API zend_attribute *zend_add_property_attribute(zend_class_entry *ce, zend_property_info *info, zend_string *name, uint32_t argc);
ZEND_API zend_attribute *zend_add_class_constant_attribute(zend_class_entry *ce, zend_class_constant *c, zend_string *name, uint32_t argc);

And provide implementations like that in zend_attributes.c:

static void attribute_ptr_dtor(zval *v)
{
	zend_attribute_free((zend_attribute *) Z_PTR_P(v), 1);
}

ZEND_API zend_attribute *zend_add_class_attribute(zend_class_entry *ce, zend_string *name, uint32_t argc)
{
    zend_bool persistent = (ce->type == ZEND_INTERNAL_CLASS);

    if (ce->attributes == NULL) {
        ce->attributes = pemalloc(sizeof(HashTable), persistent);
        zend_hash_init(ce->attributes, 8, NULL, attribute_ptr_dtor, persistent);
    }

    zend_attribute *attr = pemalloc(ZEND_ATTRIBUTE_SIZE(argc), persistent);
    attr->name = zend_string_dup(name, persistent);
    attr->lcname = zend_string_tolower_ex(attr->name, persistent);
    attr->offset = 0;
    attr->argc = argc;

    zend_hash_next_index_insert_ptr(ce->attributes, attr);

    return attr;
}

@kooldev That looks reasonable to me. Will probably make the compiler code a bit uglier...

attr->name = zend_string_dup(name, persistent);

I'd instead assert that the input has the right persistence type.

CI is green, the Azure failure is unrelated.

Merged in a7908c2