Skip to content

Allow INCLUDEJS to include files outside the style directory#880

Closed
Fyorl wants to merge 1 commit into
phpbb:developfrom
Fyorl:ticket/10944
Closed

Allow INCLUDEJS to include files outside the style directory#880
Fyorl wants to merge 1 commit into
phpbb:developfrom
Fyorl:ticket/10944

Conversation

@Fyorl

@Fyorl Fyorl commented Jul 6, 2012

Copy link
Copy Markdown
Contributor

…ate dir

Also added type="text/javascript" to the generated script tag.

PHPBB3-10944
@travisbot

Copy link
Copy Markdown

This pull request fails (merged 3fc2312 into d9fd0cc).

@naderman

naderman commented Jul 6, 2012

Copy link
Copy Markdown
Member

Ok two things: Why only {FOO}/x/y and not x/y{FOO}/{BAR}? And please add tests for this.

@naderman

naderman commented Jul 6, 2012

Copy link
Copy Markdown
Member

Oh and please enter your pull request as a proposed patch in the ticket so one can find it from there as well.

@naderman

naderman commented Jul 6, 2012

Copy link
Copy Markdown
Member

Actually the whole deal about allowing variables in paths to include seems to be an entirely unrelated separate issue? Can't you just create a ticket for that and deal with it separately?

@naderman

naderman commented Jul 6, 2012

Copy link
Copy Markdown
Member

Same thing actually applies for the type="text/javascript" bit. It's not necessary in HTML5. If you want to add it back please create a ticket that explains why, and do so in a separate PR.

@Fyorl

Fyorl commented Jul 7, 2012

Copy link
Copy Markdown
Contributor Author

Yes, there are two different issues here, it just so happens that allowing for {FOO}/a/b allows INCLUDEJS to include scripts outside the style's template directory. That's why the two issues are connected here. However, I've done some more testing and it seems there is at least one other code path that can be taken that still prepends the style's template path erroneously. I think it best if you could close this PR (again, sorry >_<) and I will create another ticket for the {FOO}/a/b issue but restrict this ticket to fixing the other code path.

@naderman

naderman commented Jul 7, 2012

Copy link
Copy Markdown
Member

Alright, no problem.

@naderman naderman closed this Jul 7, 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

Development

Successfully merging this pull request may close these issues.

3 participants