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

Added the explanation about addClassesToCompile() method #6405

Closed
wants to merge 6 commits into from

Conversation

javiereguiluz
Copy link
Member

This fixes #4492.


The main drawback of this technique is that it doesn't work for classes which
contains annotations, such as controllers with ``@Route`` annotations and
entities with ``@ORM`` or ``@Assert`` annotations.
Copy link
Member

Choose a reason for hiding this comment

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

classes containing usages of __DIR__ and __FILE__ cannot be added either (as loading them from classes.php would change these paths).

You should also ad a note saying that adding a class will automatically add any parent class

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent warnings! Thank you. I've added them and reworded the rest a bit.

* Classes contain annotations, such as controllers with ``@Route`` annotations
and entities with ``@ORM`` or ``@Assert`` annotations;
* Classes use the ``__DIR__`` and ``__FILE__`` constants, because their values
will change when loading these classes from the ``classes.php`` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would find it interesting to know why it doesn't work for annotations. One of the reasons is the file location retrieved from reflection. Is this something you could add to the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just added your proposal. Thanks!

@javiereguiluz
Copy link
Member Author

This PR is read to be merged too. Thanks!

After adding to compile all the classes commonly used by your bundle, you can
expect a minor performance improvement.

Beware that this technique can't be used in some cases:
Copy link
Member

Choose a reason for hiding this comment

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

I would put this one and the lines below in a caution block to emphasize it a bit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead I've added bold text to emphasize. Personally I don't like to define so many admonitions. It "breaks" the reading flow.

@javiereguiluz
Copy link
Member Author

I've simplified everything to explain things better.

@wouterj
Copy link
Member

wouterj commented May 21, 2016

👍

1 similar comment
@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

👍

@wouterj
Copy link
Member

wouterj commented May 21, 2016

Thanks Javier, looks great!

wouterj added a commit that referenced this pull request May 21, 2016
…d (javiereguiluz)

This PR was squashed before being merged into the 2.3 branch (closes #6405).

Discussion
----------

Added the explanation about addClassesToCompile() method

This fixes #4492.

Commits
-------

c5664a1 Added the explanation about addClassesToCompile() method
@wouterj wouterj closed this May 21, 2016
@javiereguiluz javiereguiluz deleted the fix_4492 branch May 24, 2018 16:04
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.

5 participants