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] Add view path to end of compiled blade view #28117

Merged
merged 1 commit into from
Apr 5, 2019
Merged

[5.8] Add view path to end of compiled blade view #28117

merged 1 commit into from
Apr 5, 2019

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Apr 4, 2019

This re-adds the functionality that was added in #27544 and #27976 and removed again in 33ce7bb.

The main difference with this approach is that it takes into account the issue from #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.

@bzixilu this would provide a "unique" way for PHPStorm to pick up the path. I suspect changes on PHPStorm's end are needed to make this work. But you could, in theory, always rely on the path being between the /**PATH & ENDPATH**/ comment blocks. Would this approach help?

Replaces #28099

This re-adds the functionality that was added in #27544 and #27976 and removed again in 33ce7bb.

The main difference with this approach is that it takes into account the issue from #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.
@@ -122,6 +122,10 @@ public function compile($path = null)
$this->files->get($this->getPath())
);

if (! empty($this->getPath())) {
$contents .= "<?php /**PATH {$this->getPath()} ENDPATH**/ ?>";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$contents .= "<?php /**PATH {$this->getPath()} ENDPATH**/ ?>";
$contents .= "<?php #PATH {$this->getPath()} ENDPATH# ?>";

Maybe just a slightly different comment syntax for aesthetics and shorter regex pattern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be possible yes. Gonna wait what @bzixilu says.

@fitztrev
Copy link
Contributor

fitztrev commented Apr 4, 2019

Was the cache filename as a base64 encoded string instead of a sha1 (suggested by Taylor) decided against?

@driesvints
Copy link
Member Author

@fitztrev it's still on the table but in my eyes a more overkill solution than the simple solution this PR provides.

@bzixilu
Copy link
Contributor

bzixilu commented Apr 5, 2019

Hi all, thank you for taking care of the debug feature.

Both suggested variants - encoding of a view path into a compiled view filename as well as the movement of the debug info to the line above - will work to provide a debug info to PhpStorm since we can in both cases somehow retrieve a view path from a compiled view. Each of the proposed solutions requires changes from our side that we will be able to provide only in PhpStorm 2019.1.1. But that is ok. Completely losing the feature is much worst.

Below are a couple of thoughts about base64 encoding.

Base64 encoding works in both directions, so in the future, we will be able to provide the additional info, e.g., line mappings that can also be easily decoded for the debug needs (I would avoid one-way encryption for our purposes).

But there are a few things that should be taken into account if you choose this way:

  1. Length of a message encoded with base64 depends on initial message length (1.3 times longer). If we want to provide more info in the future, it may lead to exceeding the limit on the length of the filename in Windows, so the length of the message should be under control.

  2. The standard base64 encoding may use "/" sign in the encoded message, but it is forbidden sign in the filename in Linux and Windows. So some modified base64 encoding should be considered.

Maybe there is something else that didn't come up to my mind.

To summarize, I would vote for the human-readable comment like you've implemented in this PR.

@sisve
Copy link
Contributor

sisve commented Apr 6, 2019

This looks like it produces invalid files for blade views ending with an open php block. I believe the fix is to check if there's an open php block, and close it before appending the path comment.

To be clear, here's an example file of what I mean:

<?php echo $html

This file becomes

<?php echo $html<?php /**PATH foo ENDPATH**/ ?>

@driesvints driesvints deleted the re-introduce-compiled-view-paths branch April 8, 2019 09:58
@driesvints
Copy link
Member Author

@sisve thanks for making me aware. I've spent some time today working on a fix: #28141

taylorotwell pushed a commit to illuminate/view that referenced this pull request Apr 10, 2019
When appending the view path to Blade views with only one line and opening tags we should take into consideration for closing the PHP tag first before appending the path. I've addeded tests which should cover quite a few situations.

Thanks to Sisve for making me aware of this: laravel/framework#28117 (comment)
taylorotwell pushed a commit to illuminate/view that referenced this pull request May 11, 2021
When appending the view path to Blade views with only one line and opening tags we should take into consideration for closing the PHP tag first before appending the path. I've addeded tests which should cover quite a few situations.

Thanks to Sisve for making me aware of this: laravel/framework#28117 (comment)
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.

5 participants