Hi there,
Am 24.04.2019 um 21:07 schrieb Peter Kokot:
Hello,
On Wed, 24 Apr 2019 at 19:44, Chase Peeler <chasepee...@gmail.com> wrote:
On Wed, Apr 24, 2019 at 1:27 PM Marco Pivetta <ocram...@gmail.com> wrote:
Run a fixer: they are out there, and they are extremely stable too.
Also a good chance to finally take a look at code that has been rotting in
a hard drive for too much time.
All of that takes time though. I have 6,787 short opening tags found. Even
if I use a fixer to generate a diff, or, to fix them and then examine the
diff in a pull request... that's going to take a LOT of time. It's going to
start getting messy if I find false positives and need to exclude changes.
It still doesn't address the impact of changes that aren't found. Are you
100% positive that the fixers out there will catch EVERY single instance?
php-cs-fixer doesn't update <? inside of strings. If that's generating
dynamic code, then that's not getting caught.
The output from php-cs-fixer just generated a diff file that was 161,000
lines long. But yea, that's something I can scan through real quick and
make sure nothing is going to get broken. No chance I'll miss something
either.
I would LOVE to have the time to go through our legacy code and clean out
old stuff. We have a lot of technical debt that, while we aren't paying it
off at the moment, it's gaining any interest either. We also have plenty
that is. It's tough to justify putting off the stuff that is gaining
interest to focus on stuff that isn't so we can fix short tags.
I don't work for a software company. I develop an internal application for
a non-software company. There are 2 other developers and enough work for 10
developers. It's going to be VERY hard for me to get management to approve
putting off projects that have a direct impact on our business in order to
upgrade to PHP 8 when that time comes. I think you are going to find that's
a pretty common occurrence, and the adoption rate of PHP 8 is going to be
VERY low. Especially when more and more user-friendly alternatives like
python and node are coming along. It was one thing when the options were
RoR, ASP.NET, and JSP. That's not the ecosystem anymore, and it's only
going to provide additional user friendly opportunities in the next couple
of years leading up to PHP8.
Can someone PLEASE tell me the positive gains this RFC will accomplish that
justifies risking:
1.) Losing market share
2.) losing credibility
3.) removing a large number of libraries that are fine from a technical
aspect, but will stop working due to the existence of <?
4.) new security risks (code leaks that are more likely than the code leaks
the RFC seeks to prevent)
And, please don't use the existence of code fixers to justify this unless
you are willing to demonstrate how it's quick and easy to go through a
161,000 line diff file or are willing to 100% guarantee that the fixer will
not break anything, will not fix anything it shouldn't, and will not miss
anything it should fix.
--
Chase Peeler
chasepee...@gmail.com
I've done few commits with about 8k files changed with very repeating
changes like this and without breaking anything. It will take you
about 30minutes... Let's say, one hour for also taking a break from
all the scrolling the git diff and to return the merge the next day
after another check or something like this.
The change (speaking for 8k files using short opening tags converted
to <?php) itself takes about 80 seconds with using PHP CS Fixer but
the memory_limit needs to be increased a bit and only relevant change
is done in all files. 8k (or 6k) opening tags is nothing very big
actually.
Then also search for similar occurrences by using a regex for those
edge cases you're mentioning in strings and such (I doubt there are
but ok) and you have this fixed. Then run this app on PHP 8 and you'll
see the other BC breaks that happen due to the major release. Major
releases are meant to break things because otherwise we can only load
the language with new things and maintain old stuff in there for
eternity.
That's why serious apps have support cycles and follow their
frameworks release and life cycle, and in the end also PHP cycle.
Simple as that.
--
Peter Kokot
Some thoughts and numbers from my side.
Nobody should not use a simple grep or sed like this:
grep -rin "<?[^pxi=]" . | grep ".php"
It does not search in .phtml templates for example. And it has lots of
false-positives.
This is the output in one of my projects:
# grep -rin "<?[^pxi=]" . | grep ".php"
./scripts/OpenInviter/autoupdate.php:97:
$start=0;$end=strpos($res,"<?");$signatureBulk=trim(substr($res,$start,$end));$fileStriped=str_replace($signatureBulk,'',$res);
./scripts/XXXX_soap/XXXX_soap.php:1:<?
./scripts/XXXupload.php:67: Text: <input type="text" name="text"
value="<?echo $_REQUEST['text']?>"><br>
./scripts/XXXupload.php:68: Text2: <input type="text"
name="text2" value="<?echo $_REQUEST['empfaenger']?>"><br>
Binary file ./library/phpillow/tests/phpillow/data/image_gif.gif matches
Binary file
./library/phpillow/tests/phpillow/data/.svn/text-base/image_gif.gif.svn-base
matches
./library/ezcomponents/Mail/src/tools.php:225: $pattern =
'/<?\"?[a-zA-Z0-9!#\$\%\&\'\*\+\-\/=\?\^_`{\|}~\.]+\"?@[a-zA-Z0-9!#\$\%\&\'\*\+\-\/=\?\^_`{\|}~\.]+>?$/';
./library/XXXX/SoapApi.php:1:<?
Binary file
./library/dompdf/lib/php-font-lib/sample-fonts/IntelClear-Light.ttf matches
Result:
2 false positives
3 results because "<?" seems to be in random binary files, and "php" is
in the file path. Better check would be:
grep -rin "<?[^pxi=]" . | grep "\.php"
2 places found where "<?echo " should be replaced by "<?=" or "<?php "
2 results with a short open tag at the beginning of a file
BUT: That simple grep did only find a small fraction of the problems,
for example .phtml templates were not scanned. Continue reading...
php-cs-fixer with default settings produced a diff file with 3403 lines,
51 line changes:
php php-cs-fixer-v2.phar fix --rules=full_opening_tag --diff --dry-run .
> result.txt
Reviewing these 51 changes took 3 minutes. No false positives here, all
of them were at the beginning of files.
BUT: it didn't search in .phtml files. php-cs-fixer by default only
processes .php and .phpt files (depending on the version you are using,
older versions also process .twig files as far as I can see). If you use
it, you may have to add other file extensions like .phtml, .tpl or
similar, depending on your project.
Running php-cs-fixer on .phtml files in that project results in a diff
file with 9527 lines, 1573 line changes just in the .phtml files of that
project. Whao...
I'm not sure if I'm alone, but I have lots of template code like this:
<div class="tableBody">
<? foreach ($this->Xdata as $index => $data) { ?>
<div class="tableEntry <? if ($index%2 != 0) echo
'odd';?>">Something</div>
<? } ?>
</div>
Blindly replacing "<? " by "<?php " results in this:
<div class="body">
<?php foreach ($this->Xdata as $index => $data) { ?>
<div class="entry <?php if ($index%2 != 0) echo
'odd';?>">Something</div>
<?php } ?>
</div>
The nice visual indenting of the "foreach" with 4 spaces is broken now,
and cannot be fixed, because "<?php " is 6 chars long...
Yes, minor problem of cause, but it looked nice before, and now looks
broken and "wrongly indented".
Some random thoughts:
- What happens to .phar files? Do we have to scan the contents?
- What happens to "executable php files"? They don't have a file
extension, but a shebang "#!/usr/bin/env php", and potentially
short-open-tags in it.
- What happens if I miss to add a file extension to php-cs-fixer while
scanning and fixing code I'm not 100% familiar with? Code leak :-(
- What happens if I upgrade a server to PHP8, but do not check all
folders and projects on it?
- What happens if a Shared Hosting Provider increases the PHP version to
8 automatically for all its customers in the future? Does this happen in
the real world? Pages will not generate a 500 error, they will leak
code, with passwords in it...
Same with system administrators upgrading PHP to 8 without asking
code-owners/developers. Or migrating all projects blindly to a new
server with PHP8 installed. I guess that might happen. I guess, nobody
expects code leaks then...
Looks like a lot of work and problems for everybody... I didn't expect
the RFC to pass, thought that there are too many problems and lots of
work for no benefit :-( And I don't follow internals mailing list on a
daily basis, just check it "every few weeks for something important"...
In the past I was fine with BC breaks, for security, performance, or
similar. Even bigger ones (unable to detect like "Uniform Variablen
Syntax" for example). But this one looks heavy and dangerous for no
benefit...
At least the code leaks must not happen. A Fatal error is a lot better
than a code leak (and easier to find in the error log :-) )...
"Just use php-cs-fixer" sounds easy, but it's a lot more complex, see
problems and thoughts above.
Michael
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
