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

[ticket/10799] Removing global variable from includejs #787

Merged
merged 1 commit into from Jun 12, 2012
Merged

[ticket/10799] Removing global variable from includejs #787

merged 1 commit into from Jun 12, 2012

Conversation

cyberalien
Copy link
Contributor

Removing global $phpbb_root_path from includejs implementation

http://tracker.phpbb.com/browse/PHPBB3-10799

@@ -48,7 +48,7 @@ class phpbb_template
* phpBB root path
* @var string
*/
private $phpbb_root_path;
public $phpbb_root_path;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why does this need to become public? Can't _js_include just prefix it's parameter with $this->phpbb_root_path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$this refers to template renderer instance, not template.

I can add phpbb_root_path to renderer and then change it to $this->phpbb_root_path. Would it be a better solution than changing template's phpbb_root_path to public?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think you misunderstood what I meant. Shouldn't the _js_include() function simply add $this->phpbb_root_path in front? But I guess it's cleaner if that function can take any path and won't always prepend the root path.

But I would definitely prefer that you pass $phpbb_root_path into the renderer over the current public solution.

Removing global $phpbb_root_path from includejs implementation

PHPBB3-10799
@cyberalien
Copy link
Contributor Author

Changed. Moved it to _js_include() function, so it could use template's phpbb_root_path variable

@naderman
Copy link
Sponsor Member

naderman commented May 3, 2012

Looks good :)

p added a commit to p/phpbb3 that referenced this pull request Jun 12, 2012
* cyberalien/ticket/10799:
  [ticket/10799] Removing global variable from includejs
@p p merged commit 63b4191 into phpbb:develop Jun 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants