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

Go 1.7: {{if}} branches end in different contexts #2331

Closed
strk opened this issue Aug 18, 2017 · 38 comments · Fixed by #2336
Closed

Go 1.7: {{if}} branches end in different contexts #2331

strk opened this issue Aug 18, 2017 · 38 comments · Fixed by #2336
Labels
issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself type/bug
Milestone

Comments

@strk
Copy link
Member

strk commented Aug 18, 2017

As of commit e08d1fc (current master branch) new installation fails with following error:

html/template:user/dashboard/repo-search:3:13: {{if}} branches end in different contexts: {stateJSRegexp delimNone urlPartNone jsCtxRegexp attrNone elementScript }, {stateJS delimNone urlPartNone jsCtxRegexp attrNone elementScript }

Gitea has been built with go build.
The offending route is the default one (/) which is were you are directed right after install.

@lafriks
Copy link
Member

lafriks commented Aug 18, 2017

strange, I can't seem to be able to reproduce that. Not in new install, not in existing one. Built with TAGS="sqlite" make build

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Here I have go version go1.7.4 linux/amd64

The other specificity of my install is that I'm using an existing PostgreSQL database, with a user in it.
When cleaning cookies the error appears after login.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

I can still reproduce with TAGS="sqlite" make build builds (and sqlite3 database).
In this case I have to register a new user before logging in, and get that error upon first login.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

The template does indeed look weird, check this snippet:

        <div v-if="tab === 'repos'" class="ui tab active list">

What's that v-if ??

@daviian
Copy link
Member

daviian commented Aug 18, 2017

@strk It's from Vue.js. Do you use Chrome? Chrome has an intriguing caching now. Open your public/index.js in a seperate tab and reload. Maybe you have an old js file.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Ok, anyway the problem is somewhere in here, at the top of templates/user/dashboard/repo-search.tmpl:

        {{if not .ContextUser.IsOrganization}}
        <div class="ui two item stackable tabable menu">
            <a :class="{item: true, active: tab === 'repos'}" @click="changeTab('repos')">{{.i18n.Tr "repository"}}</a>
            <a :class="{item: true, active: tab === 'orgs'}" @click="changeTab('orgs')">{{.i18n.Tr "organization"}}</a>
        </div>
        {{end}}

Removing the portion inside the condition, leaving the following, fixes it:

        {{if not .ContextUser.IsOrganization}}
        {{end}}

@daviian
Copy link
Member

daviian commented Aug 18, 2017

I can't reproduce that error as well. I am on latest master. My Go version is 1.8.3

@strk
Copy link
Member Author

strk commented Aug 18, 2017

What Go version are you using ? What else could be different ?

@daviian
Copy link
Member

daviian commented Aug 18, 2017

As I said, Go version is 1.8.3
Have you tried hard resetting your git repository to remove old folders etc.? Just an idea.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

@lafriks are you also using Go 1.8 ? Both: any chance you can try an older version ?
I'll repeat the steps to reproduce:

  1. Install Gitea (tried both PostgreSQL and Sqlite3 backends)
  2. Register new account (will become admin)
  3. Login as the new account

@daviian
Copy link
Member

daviian commented Aug 18, 2017

@strk I will try with an older version, give me a moment.

@daviian
Copy link
Member

daviian commented Aug 18, 2017

Ok I can confirm this issue with Go 1.7.4!

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Thanks for testing ! Should this be filed upstream to Vue ? (or can you tell where else the bug is ?)

@strk strk changed the title {{if}} branches end in different contexts Go 1.7: {{if}} branches end in different contexts Aug 18, 2017
@daviian
Copy link
Member

daviian commented Aug 18, 2017

I don't think Vue is responsible for this. I'd rather say to file an issue at Golang for the bug in 1.7 ;-)

@strk
Copy link
Member Author

strk commented Aug 18, 2017

From https://golang.org/pkg/html/template/:

        // ErrBranchEnd: "{{if}} branches end in different contexts"
        // Example:
        //   {{if .C}}<a href="{{end}}{{.X}}
        // Discussion:
        //   Package html/template statically examines each path through an
        //   {{if}}, {{range}}, or {{with}} to escape any following pipelines.
        //   The example is ambiguous since {{.X}} might be an HTML text node,
        //   or a URL prefix in an HTML attribute. The context of {{.X}} is
        //   used to figure out how to escape it, but that context depends on
        //   the run-time value of {{.C}} which is not statically known.
        //
        //   The problem is usually something like missing quotes or angle
        //   brackets, or can be avoided by refactoring to put the two contexts
        //   into different branches of an if, range or with. If the problem
        //   is in a {{range}} over a collection that should never be empty,
        //   adding a dummy {{else}} can help.
        ErrBranchEnd

Maybe we can try the dummy {{else}} trick...

@strk
Copy link
Member Author

strk commented Aug 18, 2017

dummy {{else}} does not help :(

@daviian
Copy link
Member

daviian commented Aug 18, 2017

I found this excerpt earlier today too, but thought that won't help in any way ;-)

Why not just updating Go?

@daviian
Copy link
Member

daviian commented Aug 18, 2017

There was a major change in text/template from Go 1.7.5 to 1.8.0

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Go 1.7 is what is shipped by latest Long-Term-Support popular free unix distributions.
If we drop support for 1.7 it must be properly documented (and decided).
Me, I'd rather find a workaround than dropping support for 1.7.
Check out CHANGELOG.md, the BREAKING section is where a drop of support for 1.7 would go.

@daviian
Copy link
Member

daviian commented Aug 18, 2017

Ok sorry, haven't thought about that.

@andyleap
Copy link
Contributor

Most likely the issue is golang/go@ffd1c78

As the file in question is all wrapped in a <script>, 1.7.4 and before are treating it as JS, but 1.8 sees that it's not JS. Not sure how much you can do to fix it.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Dropping the <script type="text/x-template" id="repo-search-template"> and relative closing tag prevents the error message, but doesn't give a correct rendering. Turning the type to text/template doesn't prevent the error message.

I'll note that this template is the only template using that <script> hack (which is documented elsewhere to be an hack). It was introduced with PR #2285

@daviian
Copy link
Member

daviian commented Aug 18, 2017

I think the script tag exists because it is a Vue template and not a Go html/template.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

According to https://sebastiandedeyne.com/posts/2016/dealing-with-templates-in-vue-20 even documentation discourage from using the x-template trick.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

Comment from #go-nuts IRC channel:

21:40 < Vendan> strk, it's def. a go template as well as a vue template
21:40 < Vendan> which is kinda an abomination, imo

@lafriks
Copy link
Member

lafriks commented Aug 18, 2017

@strk yes it is not best practice but currently it would be quite big task to move template out to Vue files/templates as it would need to create way to get translations to javascript + add webpack javascript build step. What could be done currently to support do not use Vue component but directly use Vue app for this. This would fix the go 1.7 problem

@lafriks lafriks added the issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself label Aug 18, 2017
@lafriks lafriks added this to the 1.2.0 milestone Aug 18, 2017
@lafriks
Copy link
Member

lafriks commented Aug 18, 2017

@Morlinest can you for now make that vue component back to vue app so not to use script template so that Gitea can be compiled with GoLang 1.7 again? After 1.2 is released we should make correct solution with component moved to vue file and webpack build step

@Morlinest
Copy link
Member

@strk @lafriks Interesting ... I can make some changes, for example use inline-template instead (or back to Vue). Can you try first if #2317 can fix it? Did you try it before x-template was used?

@Morlinest
Copy link
Member

If #2317 does not help, aslo try to include code from repo-search.tmpl directly to dashboard.tmpl.

@daviian
Copy link
Member

daviian commented Aug 18, 2017

@Morlinest The problem is the <script>-tag. As it tells the templating engine of Go 1.7 to interpret this part as JS. Mentioned in #2331 (comment)
Don't think copying the <script> part directly to dashboard.tmpl resolves the issue.

@strk
Copy link
Member Author

strk commented Aug 18, 2017

#2317 seems to fix it for me. Do you confirm @daviian ?

@lafriks
Copy link
Member

lafriks commented Aug 18, 2017

@strk it will fix template compile error but will not fix rendering on 1.7 as it will escape translations as if they are part of javascript that is not correct.

@lafriks
Copy link
Member

lafriks commented Aug 18, 2017

@Morlinest I think currently it is needed to change it back to Vue app

@daviian
Copy link
Member

daviian commented Aug 18, 2017

@strk Yes I can confirm, however there are lots of errors.

repo-search-errors

imo @lafriks is right. The best solution would be a complete webpack build chain and a clean separation between Vue templates and Go templates. I guess this is not doable for 1.2.0

@Morlinest
Copy link
Member

OK, just <script> tag is no no. I'll do something with it.

@Morlinest
Copy link
Member

Please try #2336

@daviian
Copy link
Member

daviian commented Aug 18, 2017

@Morlinest Fixes the issue 👍

@Morlinest
Copy link
Member

@daviian Great, thanks for test.

@lunny lunny added the type/bug label Aug 25, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants