Skip to content

scope isolation for user includes#2667

Merged
naderman merged 1 commit intocomposer:masterfrom
nicolas-grekas:master
Feb 5, 2014
Merged

scope isolation for user includes#2667
naderman merged 1 commit intocomposer:masterfrom
nicolas-grekas:master

Conversation

@nicolas-grekas
Copy link
Copy Markdown
Contributor

Prevents access to $this/self from included files.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need to use includeIfExists here. It will always exist as it is in the repo.

and no need to use ../src/ as __DIR__ is already the src folder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed

@naderman
Copy link
Copy Markdown
Member

naderman commented Feb 5, 2014

Build fails because of function redefinition.

@nicolas-grekas
Copy link
Copy Markdown
Contributor Author

Fixed

@naderman
Copy link
Copy Markdown
Member

naderman commented Feb 5, 2014

@stof You see any issues with this? I don't really see what the big deal with being able to access $this theoretically is, but this is fine by me too.

@stof
Copy link
Copy Markdown
Contributor

stof commented Feb 5, 2014

It looks fine to me.
The only issue I see is the pain for people who have a global composer install as well as a local one, as they would need to take care to rebuild the global autoloader to avoid issues (using the outdated global autoloader not defining the function would create weird issues if the local autoloading needing the function in autoload_real is loaded in the same process). But this is true for any upgrade of the ClassLoader (the introduction of PSR-4 for instance). I consider it is fine

@naderman
Copy link
Copy Markdown
Member

naderman commented Feb 5, 2014

Yeah I think that we can live with, so merging this.

naderman added a commit that referenced this pull request Feb 5, 2014
scope isolation for user includes
@naderman naderman merged commit aef0483 into composer:master Feb 5, 2014
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Err now that I think about it, i forgot to comment on this, why doesn't this function just have a proper parameter?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Guess I can answer that myself, to keep the scope empty :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants