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

Replace our escaper with Zend Escaper #5

Closed
lonnieezell opened this issue Nov 5, 2015 · 6 comments
Closed

Replace our escaper with Zend Escaper #5

lonnieezell opened this issue Nov 5, 2015 · 6 comments
Labels
enhancement PRs that improve existing functionalities

Comments

@lonnieezell
Copy link
Member

https://github.com/zendframework/zend-escaper

@lonnieezell lonnieezell added the enhancement PRs that improve existing functionalities label Nov 5, 2015
@jim-parry
Copy link
Contributor

+1

@sv3tli0
Copy link
Contributor

sv3tli0 commented Dec 2, 2015

I don't want to comment work which is yet under process. But because I am watching I want to comment this ComposerScripts.php#25 ..

Its really bad idea to have such direct dependency of some 3rd party library. If there is any change of this library it should not be applied automatic into the system as it can break it.

P.S. At the same time if just 1 exact version is used of this Zend why do we have to copy it from the vendor each time when composer update is runned..

@lonnieezell
Copy link
Member Author

Per composer.json, it limits it to generally non-BC breaking versions. It's always possible that there will be a break, but very unlikely, especially coming from a company like Zend.

While we won't use very many third-party libraries for the reasons you mentioned, there are times where it's extremely practical to use one, and fool-hardy not to. This is one of those cases. Proper escaping of output is an extremely complex problem. Zend has done the best job of anyone I could see out there of catching all of the edge-cases and complexities into a good library. For us to recreate that particular wheel would be silly. And dangerous to those using CI thinking it helps them be secure.

And we're not limiting to one exact version of the library. It follows semantic versioning so could be anything between 2.5 and NOT 3, so bug-fixes and enhancements can come along. That's why we check every time an update is ran and copy the new over if so.

The file is moved over, instead of leaving it in place for Composer to manage, because of one of the requirements of the framework: People have to be able to download the library and go. No Composer needed. This gets the file in place and usable without so that when releases are eventually created, everything's ready.

@jim-parry
Copy link
Contributor

"Zend\Escaper was written with ease of use in mind, so it can be used completely stand-alone from the rest of the framework, and as such can be installed with Composer using zendframework/zend-escaper."

This component is intended to be usable stand-alone, and does not bring in any of the complexity normally inherent in using third party components.

@sv3tli0
Copy link
Contributor

sv3tli0 commented Dec 2, 2015

@lonnieezell I understand your idea. How ever an auto updating Core sounds not stable !
A lot more stable will be the Update of any 3rd party component as this Zend Escapare to be made by the staff and pushed as small sub release of CI.
"The file is moved over, instead of leaving it in place for Composer to manage" - all who use composer will have auto updating Escaper on each Zend Escaper new version and the others which don't use Composer and they update manually instead, will have older Escaper version with the new CI ?

@lonnieezell
Copy link
Member Author

@sv3tli0 I understand your concerns, and it's something that we'll definitely keep an eye on and modify if needed. However, I think you'll find that as long as it's carefully done, it is fine. That's the beauty of Composer and Semantic versioning. One of the most popular PHP frameworks, Laravel, currently works this way with around 25 third-party libraries used as part of the core of the framework, all installed via Composer during install/update.

all who use composer will have auto updating Escaper on each Zend Escaper new version and the others which don't use Composer and they update manually instead, will have older Escaper version with the new CI?

More or less, yeah. With each tagged release it will have the current version of Escaper. If you use Composer, then you get bug-fixes and security fixes faster. If you're pulling from the develop repo directly through git then, yes, you run the risk of having a not up-to-date version of Escaper, but that's the risk using a 'develop' branch and not using a stable release.

Sosko added a commit to Sosko/CodeIgniter4 that referenced this issue Sep 24, 2020
If thy table haven't primary key, and i pass $returnID = false, than the ci4 throw this error:
```
CRITICAL - 2020-09-24 00:47:00 --> pg_query(): Query failed: ERROR:  lastval is not yet defined in this session
#0 [internal function]: CodeIgniter\Debug\Exceptions->errorHandler(2, 'pg_query(): Que...', '/var/www/ci4/sy...', 196, Array)
codeigniter4#1 /var/www/ci4/system/Database/Postgre/Connection.php(196): pg_query(Resource id codeigniter4#9, 'SELECT LASTVAL(...')
codeigniter4#2 /var/www/ci4/system/Database/BaseConnection.php(741): CodeIgniter\Database\Postgre\Connection->execute('SELECT LASTVAL(...')
codeigniter4#3 /var/www/ci4/system/Database/BaseConnection.php(669): CodeIgniter\Database\BaseConnection->simpleQuery('SELECT LASTVAL(...')
codeigniter4#4 /var/www/ci4/system/Database/Postgre/Connection.php(519): CodeIgniter\Database\BaseConnection->query('SELECT LASTVAL(...')
codeigniter4#5 /var/www/ci4/system/Model.php(887): CodeIgniter\Database\Postgre\Connection->insertID()
codeigniter4#6 /var/www/ci4/app/Models/MyModel.php(46): CodeIgniter\Model->insert(Array, false)
codeigniter4#7 /var/www/ci4/app/Controllers/MyController.php(113): App\Models\MyModel->new_connection('1', '1')
codeigniter4#8 /var/www/ci4/app/Controllers/MyController.php(54): App\Controllers\MyController->do_create_connection()
codeigniter4#9 /var/www/ci4/system/CodeIgniter.php(918): App\Controllers\MyController->create_connection()
codeigniter4#10 /var/www/ci4/system/CodeIgniter.php(404): CodeIgniter\CodeIgniter->runController(Object(App\Controllers\MyController))
codeigniter4#11 /var/www/ci4/system/CodeIgniter.php(312): CodeIgniter\CodeIgniter->handleRequest(NULL, Object(Config\Cache), false)
codeigniter4#12 /var/www/ci4/public/index.php(45): CodeIgniter\CodeIgniter->run()
codeigniter4#13 {main}
CRITICAL - 2020-09-24 00:47:00 --> Uncaught CodeIgniter\Format\Exceptions\FormatException: Failed to parse json string, error: "Type is not supported". in /var/www/ci4/system/Format/Exceptions/FormatException.php:9
Stack trace:
#0 /var/www/ci4/system/Format/JSONFormatter.php(71): CodeIgniter\Format\Exceptions\FormatException::forInvalidJSON('Type is not sup...')
codeigniter4#1 /var/www/ci4/system/API/ResponseTrait.php(414): CodeIgniter\Format\JSONFormatter->format(Array)
codeigniter4#2 /var/www/ci4/system/API/ResponseTrait.php(134): CodeIgniter\Debug\Exceptions->format(Array)
codeigniter4#3 /var/www/ci4/system/Debug/Exceptions.php(168): CodeIgniter\Debug\Exceptions->respond(Array, 500)
codeigniter4#4 [internal function]: CodeIgniter\Debug\Exceptions->exceptionHandler(Object(ErrorException))
codeigniter4#5 {main}
  thrown
#0 [internal function]: CodeIgniter\Debug\Exceptions->shutdownHandler()
codeigniter4#1 {main}
```
This will skip to getting inserted ID
@Muneeb1998 Muneeb1998 mentioned this issue Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

No branches or pull requests

3 participants