Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for PHP 8.0 class attributes generation #145

Open
wants to merge 4 commits into
base: 4.8.x
Choose a base branch
from

Conversation

jakublabno
Copy link

Q A
BC Break no
New Feature yes

Description

Feature that allows attribute generation for classes (methods/properties is the next step)
Either non-arguments attributes and named arguments are supported.
Nested attributes are not supported yet.
You can use it in a few ways:

  1. From plain array
ClassGenerator::fromArray([
            'name' => 'AnyClassName',
            'attribute' => [
                ['FirstAttribute', ['firstArgument' => 'abc', 'secondArgument' => 12]],
                ['FirstAttribute', ['firstArgument' => 'abc', 'secondArgument' => 13]],
                ['SecondAttribute'],
            ],
        ])
  1. From array with builder instance
/**
@ var AttributeGeneratorBuilder $builder
*/
ClassGenerator::fromArray([
            'name' => 'AnyClassName',
            'attribute' => $builder,
            ],
        ])
  1. From class reflection
$classGenerator = new ClassGenerator();
$attributeGenerator = AttributeGenerator::fromReflection($reflectionClass);

$classGenerator->setAttributes($attributeGenerator);

  1. From builder
$classGenerator = new ClassGenerator();
$builder = new AttributeGenerator\AttributeBuilder();
      $builder
          ->add('LaminasTest\Code\Generator\Fixture\AttributeGenerator\AttributeWithArguments', [
              'stringArgument' => 'any string',
              'intArgument' => 1,
              'boolArgument' => true,
          ])
          ->add('LaminasTest\Code\Generator\Fixture\AttributeGenerator\SimpleAttribute');

      $builder = AttributeGenerator::fromBuilder($builder);
      $generator->setAttribute($builder);

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Did a first skim: endorse simplifying and removing flexibility in a first step.

The factory and the builder are not necessary, if the attribute generator can act as an immutable builder (most common use-case anyway)

use Laminas\Code\Generator\AttributeGenerator\AttributeBuilder;
use ReflectionClass;

class AttributeGenerator extends AbstractGenerator
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this final, since it's a new symbol, and inheritance abuse is strong :-)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I don't know why I extended it, maybe in the beginning when I didn't know the code, I'll stay on with interface GeneratorInterface :)

src/Generator/AttributeGenerator/AttributeAssemblerBag.php Outdated Show resolved Hide resolved

use Stringable;

interface AttributeAssembler extends Stringable
Copy link
Member

Choose a reason for hiding this comment

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

Let's not make this Stringable: Stringable is a terrible API.

See also ShittySoft/symfony-live-berlin-2018-doctrine-tutorial#3 (comment) for more clarity

use ReflectionAttribute;
use ReflectionClass;

final class AttributeAssemblerFactory
Copy link
Member

Choose a reason for hiding this comment

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

This code can all be inlined into the AttributeGenerator: no need to make a separate class out of it.


final class AttributeBuilder
{
private array $definitions = [];
Copy link
Member

Choose a reason for hiding this comment

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

This type is to be refined

namespace Laminas\Code\Generator\AttributeGenerator;

//TODO Enum in PHP8.1
final class AttributePart
Copy link
Member

Choose a reason for hiding this comment

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

Let's mark all internally used symbols as @internal, to allow refactoring/rewrite as soon as we are allowed to.

@jakublabno
Copy link
Author

@Ocramius

@Ocramius
Copy link
Member

Sorry, didn't get to work on this at all, due to other priorities.

@Ocramius Ocramius added this to the 4.8.0 milestone Nov 18, 2022
@Ocramius Ocramius self-assigned this Nov 18, 2022
return new self(...$assemblers);
}

public static function fromArray(array $definitions): self
Copy link
Member

Choose a reason for hiding this comment

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

I decided to drop all ::fromArray() methods as per #153

This API will be dropped once I have the green light on #153

@Ocramius Ocramius changed the base branch from 4.7.x to 4.8.x December 8, 2022 01:53
@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

After digging further into this, I decided to skip it for 4.8.0.

Specifically, I would simplify the AttributeGenerator to use a ValueGenerator or similar internally, instead of the current AttributeAssembler logic.

I understand that you wanted to split generator and data structure, but practically, it doesn't really help with the use-case of this library, IMO.

What I'd end up with in the end is probably a single class with a thin public API, and a lot of private and @internal API to start with:

  • AttributeGenerator::fromReflection(Reflector): list<AttributeGenerator> (unsure about it - the API seems a bit messy)
  • AttributeGenerator::fromValue(object $attribute): self
  • AttributeGenerator#generate()

Everything else would be private or @internal in a first iteration.

Therefore needing to put this patch aside: it needs a good weekend of thinking it through, which I didn't manage to find, sorry.

@derrabus
Copy link
Contributor

derrabus commented Mar 9, 2023

Hello. 👋🏻

Is there anything I can do to help moving this PR forward? I'm working on a small code generator tool at the moment and I need the ability to emit class and property attributes.

@jakublabno
Copy link
Author

Hello. 👋🏻

Is there anything I can do to help moving this PR forward? I'm working on a small code generator tool at the moment and I need the ability to emit class and property attributes.

Sorry, I have no time to work it out, I'll try to close this next month.

@kitsunet
Copy link

If there is no objections I would pick this up, make the requested changes and see to add method support along the way.

@jakublabno
Copy link
Author

  • AttributeGenerator::fromValue(object $attribute): self

I don't understand this object type parameter, there is AttributePrototype for it
I haven't used ValueGenerator 'cause this class is too complex and terrible to maintain

@kitsunet I've pushed some changes

@jakublabno jakublabno requested a review from Ocramius June 13, 2023 10:34
@kitsunet
Copy link

Cool, thanks! I guess I'll wait for ocramius verdict then and start in parallel to create changes for class members and methods on top of this.

@jakublabno
Copy link
Author

I've already started working on methods 😱

@tyteen4a03
Copy link

Hi, any updates on this?

@jakublabno
Copy link
Author

Hi, any updates on this?

I'm waiting for @Ocramius review

@birb2345345
Copy link

@Ocramius ??

@brzuchal
Copy link

This PR is already 17mo old almost 1 and a half years, still waiting for @Ocramius re-review for 7mo now.
Can there be anyone else doing the CR?
Attributes have existed in PHP since 8.0 which is so old that not even supported anymore.
I understand someone may not have enough time, but at least please help to find another person to do the code review.

@jakublabno
Copy link
Author

@Ocramius

@lisachenko
Copy link
Contributor

Colleagues, what the current status of this PR, as I can see, this feature is awaited by many developers.

@jakublabno
Copy link
Author

I'm still waiting for @Ocramius review

@jakublabno
Copy link
Author

Is there anyone else that could review this?

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.

None yet

9 participants