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

[5.8] Fix adding extra space/line that breaks content #28099

Closed
wants to merge 5 commits into from

Conversation

bzixilu
Copy link
Contributor

@bzixilu bzixilu commented Apr 3, 2019

The pull-request contains the fix for #27996

The main idea is to remove extra line break related to the added path comment during view rendering.
This fix should not affect any line breaks which doesn't relate to added path comment.

I personally don't like the following things in the fix.

  1. If we change a form which is used to represent a path in compiled view, we should not forget to fix a rendering as well (I couldn't write a test for it, it looked quite complicated)

  2. Totally we pass through a view 2 times, the first time when including the compiled view, and the second time when checking if it ends with path comment, probably it could be optimized and done with a single pass.

I appreciate if you could advise me how to improve it.

@taylorotwell
Copy link
Member

I'm not sure this is worth fixing? Should one blade component really be dependent on there not being any space rendered after another Blade component?

@fitztrev
Copy link
Contributor

fitztrev commented Apr 3, 2019

@taylorotwell IMO yes - here's the simplest illustration why:

This blade code renders a partial with an inline element:

@each('widget', $widgets, 'widget')

Before 5.8:

Screen Shot 2019-04-03 at 10 10 11 AM

Since 5.8:

Screen Shot 2019-04-03 at 10 10 22 AM

There are times when whitespace in HTML matters.

@taylorotwell taylorotwell reopened this Apr 4, 2019
@taylorotwell
Copy link
Member

Rather than wrangling around with the file contents themselves, maybe there is a simpler way to approach this? Could we encode the original file path in the cached file path, perhaps where the cached file path is a base64_encoded version of the original file path? Or some approach like that. For now, I have disabled this feature on 5.8.

@taylorotwell taylorotwell reopened this Apr 4, 2019
@driesvints driesvints changed the title #27996 @include adding extra space/line break to content [5.8] @include adding extra space/line break to content Apr 4, 2019
@driesvints driesvints changed the title [5.8] @include adding extra space/line break to content [5.8] Fix adding extra space/line that breaks content Apr 4, 2019
@taylorotwell
Copy link
Member

Closing since #28117 was merged.

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