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

API: <% loop %> and <% with %> only ever create one new scope level (fixes #4015) #8148

Merged

Conversation

kinglozzer
Copy link
Member

Removes up an age-old quirk where every . in <% loop $Foo.Bar.Baz %> would require an extra $Up to “escape” from. That was actually a feature, but I’m yet to see compelling use-case for it and it keeps causing confusion (see #4015).

Yes this will probably cause breakages for people upgrading, but they’re going to be easy ones to fix. I’ve changed the user_error() to an exception, because that way it’s at least possible to see the name of the template that’s affected (even if only the “compiled” line number is shown).

3.x and 4.x:

{$Title}		<!-- Current page title -->
<% loop $Children.Limit(5) %>
    {$Title		<!-- Child page title -->
    {$Up.Title}		<!-- Nothing - equivalent to `$Children.Title` -->
    {$Up.Up.Title}	<!-- Current page title -->
<% end_loop %>

After this change:

{$Title}		<!-- Current page title -->
<% loop $Children.Limit(5) %>
    {$Title		<!-- Child page title -->
    {$Up.Title}		<!-- Current page title -->
    {$Up.Up.Title}	<!-- Exception: Up called when we're already at the top of the scope -->
<% end_loop %>

@tractorcow
Copy link
Contributor

I need to merge up fixes for master behat.

@tractorcow tractorcow closed this Jun 8, 2018
@tractorcow tractorcow reopened this Jun 8, 2018
@tractorcow
Copy link
Contributor

I've merged up a couple fixes for both testsession + behat-extension. Let's see if that gets the tests running properly.

@@ -192,7 +192,7 @@ public function obj($name, $arguments = [], $cache = false, $cacheName = null)
switch ($name) {
case 'Up':
if ($this->upIndex === null) {
user_error('Up called when we\'re already at the top of the scope', E_USER_ERROR);
throw new \LogicException('Up called when we\'re already at the top of the scope', E_USER_ERROR);
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 should probably remove that E_USER_ERROR...

@kinglozzer kinglozzer force-pushed the scope-creepin-while-u-sleepin branch from d7ceb18 to f188a6a Compare June 8, 2018 08:28
@kinglozzer kinglozzer force-pushed the scope-creepin-while-u-sleepin branch from f188a6a to 1ec0580 Compare June 8, 2018 08:31
@tractorcow tractorcow closed this Jun 10, 2018
@tractorcow tractorcow reopened this Jun 10, 2018
@tractorcow
Copy link
Contributor

I've merged up all the branches for core modules to master. Let's see if this gets the tests green.

@tractorcow
Copy link
Contributor

Tests are green and the code looks ok, but I really need to do some testing and research to check there are no regressions.

@tractorcow
Copy link
Contributor

FYI I used this template to test and it appeared to produce much more expected output.

<% include SideBar %>
<div class="content-container unit size3of4 lastUnit">
	<article>
		<h1>$Title</h1>
		<div class="content">$Content</div>
        <% if $Parent %>
            <h2>all siblings</h2>
            <% loop Parent.Children %>
                <div>Menu: $Title, Up title: $Up.Title</div>

                <h3>Nested</h3>
                <% loop Parent.Children %>
                    <div>Menu: $Title, Up title: $Up.Title, Up up title: $Up.Up.Title</div>
                <% end_loop %>
                <h3>end nested</h3>
            <% end_loop %>
            <h2>first sibling</h2>
            <% loop Parent.Children.Limit(1) %>
                <div>Menu: $Title, Up title: $Up.Title</div>
            <% end_loop %>
        <% end_if %>
	</article>
</div>

@tractorcow
Copy link
Contributor

The logic goes over my head a little, but the effect seems to have resolved the usability issue. I'm inclined to merge this and hope that the future will be kind. ;)

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.

2 participants