Skip to content

Conversation

@Assimilationstheorie
Copy link
Contributor

Who write this Code? No Exception Handling, no Validation.
Don't use short opener for examples. php.ini is not unique configured!
Add simple HTML-Required-Attr.

Who write this Code? No Exception Handling, no validation.
Don't use short opener for examples. php.ini is not unique configured!
Add simple HTML-Required-Attr.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 9, 2018
Copy link

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

Thanks!

<a class='login' href='<?php echo $authUrl; ?>'>Connect Me!</a>
</div>
<?php elseif (empty($short)): ?>
<form id="url" method="POST" action="<?= htmlspecialchars($_SERVER['PHP_SELF']); ?>">

This comment was marked as spam.

<input type="hidden" name="csrf_token" value="<?php echo getCsrfToken() ?>" />
<input type="submit" value="Shorten">
</form>
<form id="logout" method="POST" action="<?= htmlspecialchars($_SERVER['PHP_SELF']); ?>">

This comment was marked as spam.

@Assimilationstheorie
Copy link
Contributor Author

The Project is full of Short-Tags. I would start to fix it and rebuild the project, but im not sure if this a good idea, because we must rebuild a lot of things! What did you say? If you promise that the project will not be finished now, I will develop it until i die! :D

@tmatsuo
Copy link

tmatsuo commented Jun 11, 2018

@Assimilationstheorie I don't disagree. How do you want to proceed? Do you want to make another change on this PR and merge this first? or Do you want to file another issue and wait for us to fix them?

@Assimilationstheorie
Copy link
Contributor Author

No Issue, i fix it self. I make another PR and start this week with fix all other files. Okay? :)

@bshaffer
Copy link
Contributor

Hello, I am sorry you are not happy with the samples. But it is not necessary to remove short tags. They are 100% okay. You may be confusing them with these tags: <?, which were removed entirely in PHP 7.0. Yes, <? conflicts with XML and should always be disabled, but <?= do not, and can be used safely since PHP 5.4 (they've always been able to be used safely, but before PHP 5.4, they were bundled with <? in the same INI configuration so both had to be disabled together).

As this repo is marked as supported for PHP 5.4 and above, I do not want to change the short tags.

I see that you are also adding a placeholder for the URL, and a required tag to the input. This is nice. If you'd like to submit a separate PR with those changes, or update this PR to remove your changes to the short tags, we are happy to merge it!

@Assimilationstheorie
Copy link
Contributor Author

Hey @bshaffer

Short PHP-Tags are configured in php.ini -> "short_open_tag". Any Configuration in this File are unique and not the same for all people. When you use the "long tag", this will work on all configurations because thats the "default way". We eliminate a potential Error! :)

This was referenced Jun 13, 2018
@bshaffer
Copy link
Contributor

bshaffer commented Jun 13, 2018

@Assimilationstheorie thank you for your concern but the short_open_tag directive was removed in PHP 7.0, and was separated to not include <?= in PHP 5.4. So the only people this would cause a potential error for is people using <= PHP 5.3, which is not supported by this library.

@bshaffer bshaffer mentioned this pull request Jun 13, 2018
@Assimilationstheorie
Copy link
Contributor Author

Assimilationstheorie commented Jun 13, 2018

@bshaffer - Ahh! I see! Never stop learning! :) Thanks for this info!

I will change the short-tags and create a new PR now! :)

Update: Now #1456

@bshaffer
Copy link
Contributor

@Assimilationstheorie additionally, there is a version of this library (v1-master) which supports PHP 5.2 and 5.3, if you need those versions supported.

@bshaffer bshaffer closed this Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants