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

@include adding extra space/line break to content #27996

Closed
leafling opened this issue Mar 25, 2019 · 15 comments
Closed

@include adding extra space/line break to content #27996

leafling opened this issue Mar 25, 2019 · 15 comments
Labels

Comments

@leafling
Copy link

leafling commented Mar 25, 2019

  • Laravel Version: 5.8.7
  • PHP Version: 7.2.5, 7.3.3
  • Database Driver & Version:

Description:

Blade @includes are appending extra white space to the end

Steps To Reproduce:

test/include.blade.php
this is only a test

parent.blade.php

<div>@include('test.include')</div>

expected html output:

<div>this is only a test</div>

actual html output:

<div>this is only a test
</div>

In most circumstances, this isn't really an issue, but in cases where you need to prevent white-space between html elements, it becomes an issue as the extra whitespace creates a gap between them.

In Laravel 5.7.x, the same code returned the expected behavior. Potentially even within an earlier version of 5.8, but I'm not sure.

@driesvints
Copy link
Member

Heya, I think this might be related to #27544 which adds an extra linebreak at the end. This got fixed in #27976 which will be released tomorrow. Feel free to report back if tomorrow's release doesn't fixes your problem.

@leafling
Copy link
Author

I just updated to Laravel 5.8.8 and this issue does not appear to be resolved.

@fitztrev
Copy link
Contributor

fitztrev commented Mar 27, 2019

I'm getting the same. Here are some copy-and-paste steps to reproduce:

composer create-project --prefer-dist laravel/laravel=5.8.3 issue-27996

echo -n "<div>@include('partial')</div>" > issue-27996/resources/views/parent.blade.php
echo -n "contents" > issue-27996/resources/views/partial.blade.php
echo "Artisan::command('issue-27996', function () {
    \$this->comment(view('parent'));
});" >> issue-27996/routes/console.php

php issue-27996/artisan issue-27996

Expected vs Actual output:

- <div>contents</div>
+ <div>contents
+ </div>

@driesvints
Copy link
Member

Okay, re-opening.

@driesvints driesvints reopened this Mar 28, 2019
@driesvints driesvints added the bug label Mar 28, 2019
@driesvints
Copy link
Member

Ping @bzixilu. Do you maybe have an idea if this is related to the changes you made?

@fitztrev
Copy link
Contributor

It appears they may be related.

The cached view code for parent.blade.php:

<div><?php echo $__env->make('partial', \Illuminate\Support\Arr::except(get_defined_vars(), ['__data', '__path']))->render(); ?></div>
<?php /* /path/to/issue-27996/resources/views/parent.blade.php */ ?>

The cached view code for partial.blade.php:

contents
<?php /* /path/to/issue-27996/resources/views/partial.blade.php */ ?>

So when partial.blade.php is included, it has that new line.

@fitztrev
Copy link
Contributor

A fix may be to leave out the \n when appending the path to the compiled view.

diff --git a/src/Illuminate/View/Compilers/BladeCompiler.php b/src/Illuminate/View/Compilers/BladeCompiler.php
index 0f084b06f..c5a51c36f 100644
--- a/src/Illuminate/View/Compilers/BladeCompiler.php
+++ b/src/Illuminate/View/Compilers/BladeCompiler.php
@@ -123,7 +123,7 @@ class BladeCompiler extends Compiler implements CompilerInterface
             );
 
             if (! empty($this->getPath())) {
-                $contents .= "\n<?php /* {$this->getPath()} */ ?>";
+                $contents .= "<?php /* {$this->getPath()} */ ?>";
             }
 
             $this->files->put(

@driesvints
Copy link
Member

@fitztrev it's the base of the whole reason why the feature was added: so the path line would exist at the very last line so PHPStorm could pick it up.

@fitztrev
Copy link
Contributor

Ok, I don't know the inner workings of PHPStorm. But maybe they would be able to parse it out when it's not on its own line.

But now that you mention that, I'm curious why this was added. #27963 (comment) Maybe this should belong in a package too.

@driesvints
Copy link
Member

driesvints commented Mar 28, 2019

But now that you mention that, I'm curious why this was added. #27963 (comment) Maybe this should belong in a package too.

@fitztrev touché and you make an excellent point here. Ultimately this wasn't my call though for this feature. And I do believe that the impact of this particularly feature (the path in the view) is worth adding because it empowers such a useful debugging feature in PHPStorm.

However, I fully agree that we should look at how we can look at a way to keep the feature while solving this bug.

@driesvints
Copy link
Member

Is this only a problem when you use an include between html tags? Or also in other ways/usages?

@fitztrev
Copy link
Contributor

However, I fully agree that we should look at how we can look at a way to keep the feature while solving this bug.

👍 We'll wait to see what @bzixilu says, if there's another way PHPStorm could parse it.

Is this only a problem when you use an include between html tags? Or also in other ways/usages?

Other ways/usages too. A blade @include() is always appending a newline when rendered. The surrounding <div> tag was only used in the example to better illustrate the problem.

@devcircus
Copy link
Contributor

This was touted online by JetBrains, Taylor, Laravel News, etc. Though not specifically mentioned, it seems like it would work out-of-the-box. So including it in the core makes sense. I think we should keep trying to make this work without going the third-party route.

@bzixilu
Copy link
Contributor

bzixilu commented Mar 29, 2019

Hi all, sorry for one more breakage of Blade usage scenario. I'll start an investigation today and will do my best to come back soon with the possible solution.

bzixilu added a commit to bzixilu/framework that referenced this issue Apr 1, 2019
bzixilu added a commit to bzixilu/framework that referenced this issue Apr 2, 2019
bzixilu added a commit to bzixilu/framework that referenced this issue Apr 2, 2019
fix possible undefined offset problem
bzixilu added a commit to bzixilu/framework that referenced this issue Apr 3, 2019
bzixilu added a commit to bzixilu/framework that referenced this issue Apr 3, 2019
@driesvints
Copy link
Member

Closing this because we've decided to revert the feature for now. Please see #28099 (comment) for more info.

taylorotwell pushed a commit to illuminate/view that referenced this issue Apr 5, 2019
This re-adds the functionality that was added in laravel/framework#27544 and laravel/framework#27976 and removed again in laravel/framework@33ce7bb.

The main difference with this approach is that it takes into account the issue from laravel/framework#27996. By not introducing a newline it doesn't changes anything to the rendered view itself. The path is now applied as the final piece of code. This doesn't allows IDE's to rely on it being the last line though. Instead we use /**PATH and ENDPATH**/ to make sure we have an easy way for the IDE to pick up the path it needs.
taylorotwell pushed a commit to illuminate/view that referenced this issue May 11, 2021
This re-adds the functionality that was added in laravel/framework#27544 and laravel/framework#27976 and removed again in laravel/framework@33ce7bb.

The main difference with this approach is that it takes into account the issue from laravel/framework#27996. By not introducing a newline it doesn't changes anything to the rendered view itself. The path is now applied as the final piece of code. This doesn't allows IDE's to rely on it being the last line though. Instead we use /**PATH and ENDPATH**/ to make sure we have an easy way for the IDE to pick up the path it needs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants