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

Blade @forelse #5028

Merged
merged 1 commit into from
Jul 11, 2014
Merged

Blade @forelse #5028

merged 1 commit into from
Jul 11, 2014

Conversation

JoostK
Copy link
Contributor

@JoostK JoostK commented Jul 11, 2014

Implements #5009.

The update to a generic foreach statement may be changed, but that would need an additional keyword for this specific use-case.

I may redo this against master if requested.

@KennedyTedesco
Copy link
Contributor

👍

@taylorotwell
Copy link
Member

That was fast :)

taylorotwell added a commit that referenced this pull request Jul 11, 2014
@taylorotwell taylorotwell merged commit 2f9643a into laravel:4.2 Jul 11, 2014
@renege
Copy link

renege commented Jul 11, 2014

L3 feature back! 👍 How to use this?

@KennedyTedesco
Copy link
Contributor

@renege

@foreach($users as $user)
  <li>{{ $user->name }}</li>
@forelse
  <p>No users</p>
@endforelse

@JoostK
Copy link
Contributor Author

JoostK commented Jul 11, 2014

Now that I look at it (old documentation), I kind of prefer L3's syntax :p

@forelse ($posts as $post)
    {{ $post->body }}
@empty
    There are not posts in the array!
@endforelse

@KennedyTedesco
Copy link
Contributor

:( Hard to choose. I think @empty is more generic. I will of @forelse hahah

@Garbee
Copy link
Contributor

Garbee commented Jul 12, 2014

Um, let's not forget empty() is a PHP method. So having @empty could cause confusion. @forelse imo is much better to prevent ambiguity in what the template engine is doing.

@JosephSilber
Copy link
Member

How would this work when nested?

Wouldn't the $__empty variable be overridden?

Kinda fragile if you ask me.

@taylorotwell
Copy link
Member

Thoughts Joost?

On Jul 12, 2014, at 11:49 PM, Joseph Silber [email protected] wrote:

How would this work when nested?

Wouldn't the $__empty variable be overridden?

Kinda fragile if you ask me.


Reply to this email directly or view it on GitHub.

@JosephSilber
Copy link
Member

Maybe something like this?

protected $foreachDepth = 0;

protected function compileForeach($expression)
{
    $empty = '$__empty_' . ++$this->foreachDepth;

    return "<?php $empty = true; foreach{$expression}: $empty = false; ?>";
}

protected function compileForelse($expression)
{
    $empty = '$__empty_' . $this->foreachDepth--;

    return "<?php endforeach; if ($empty): ?>";
}

I haven't thought it through completely, but I think we could probably make it work...

@JoostK
Copy link
Contributor Author

JoostK commented Jul 13, 2014

I never thought about that, good point.

Your count based solution would not work since a foreach does not necessarily also go with the forelse, hence the counters will go out of sync. The only solution I can think of would be to use the exact L3 syntax with a different foreach keyword, so we can be sure about the counter being in sync, in which case it would work fine.

@JoostK
Copy link
Contributor Author

JoostK commented Jul 13, 2014

Actually, decrementing the counter also in compileEndforeach would allow for the current syntax.

I locally have a fix with a switch to L3's syntax, and while creating that one I thought about the aforementioned possibility to retain the syntax as it is now. Please let me know which we want to choose here, I believe there has not yet been another 4.2 tag so we may still switch I suppose.

@taylorotwell
Copy link
Member

We can still switch if it's easier. No tag yet.

On Jul 13, 2014, at 8:24 AM, Joost Koehoorn [email protected] wrote:

Actually, decrementing the counter also in compileEndforeach would allow for the current syntax.

I locally have a fix with a switch to L3's syntax, and while creating that one I thought about the aforementioned possibility to retain the syntax as it is now. Please let me know which we want to choose here, I believe there has not yet been another 4.2 tag so we may still switch I suppose.


Reply to this email directly or view it on GitHub.

@heinrichmartin
Copy link

In the end, do not forget to update docs.
@foreach is still on http://laravel.com/docs/templates#other-blade-control-structures ...

@GrahamCampbell
Copy link
Member

No it's not? The docs are already uptodate.

@JoostK
Copy link
Contributor Author

JoostK commented Aug 7, 2014

There is currently no forelse in the docs. I wanted to PR it back then but hesitated because initially the syntax had changed compared to L3. Later it has been changed into the exact L3 syntax, and it is now stable to be included in the docs I suppose.

@GrahamCampbell
Copy link
Member

@JoostK But it's right here!
yt9yfr

@JoostK
Copy link
Contributor Author

JoostK commented Aug 7, 2014

Allrighty, I was looking at the 4.1 docs. Apparently the version you see depends on the session (i.e. last used) instead of the url.

@Anahkiasen
Copy link
Contributor

Yes the url doesn't change when you switched version. Bugs me every fucking time.

@heinrichmartin
Copy link

Thanks guys! I patched my 4.2 version with this pull request as #5046 will not make it, isn't it?

@GrahamCampbell
Copy link
Member

@heinrichmartin What???

@heinrichmartin
Copy link

Sorry for the confusion. I seemingly made 2 mistakes here:

  1. Nobody here is interested in what I am doing locally (I did not commit anything).
  2. I misinterpreted Taylor's comment in the referenced pull request "Sort of prefer the original version from Joost because the opening and closing blocks have the same syntax [...]"
    What stays from my previous comment is "thank you (for taking care of this topic)". I will patiently wait for the result in the next release.

@GrahamCampbell
Copy link
Member

I will patiently wait for the result in the next release.

I still don't understand what you mean?

@heinrichmartin
Copy link

I am a user of the latest L4 release. I saw @foreach in the current doc online and tried to use it. Then it took me quite a while to understand, why I am getting PHP syntax errors. Finally, I came across the ticket and these pull requests. I wondered which one was the most promising for future releases and chose this very one to patch my private L4.

Then I wondered how to tell about the wrong doc and chose this discussion. Finally, I wanted to tell that I am happy with your activity on this project in general and this issue - and I then was surprised by your post "What???". It looked a lot like I made something wrong or stupid ... I actually do not know even now, but I wanted to let you know, that nothing bad happened to the repository.

Please let me know whether this cleared things a bit ...

@GrahamCampbell
Copy link
Member

The current 4.2 release, and the one before it supports the new blade syntax completely, and the documentation was updated almost immediately. This really needs to go no further, and should not be discussed here on github.

@laravel laravel locked and limited conversation to collaborators Aug 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants