Skip to content
This repository has been archived by the owner on Mar 5, 2022. It is now read-only.

updates related to cakephp core 4.0 release #203

Closed
wants to merge 2 commits into from
Closed

Conversation

skie
Copy link
Contributor

@skie skie commented Jan 10, 2020

No description provided.

@skie
Copy link
Contributor Author

skie commented Jan 10, 2020

@burzum, please merge this PR and tag as 3.0.0 with release initial version?

{
if (!$this->_isFileUploadPresent($entity)) {
$event->stopPropagation();

return;
return false;
Copy link
Owner

Choose a reason for hiding this comment

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

Is this actually relevant? AFAIR Cake didn't bother about any return value from a callback like beforeSave()?

Copy link
Owner

Choose a reason for hiding this comment

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

@skie any answer to this?

@@ -98,12 +99,12 @@ public function beforeMarshal(Event $event, ArrayAccess $data): void
* @param \ArrayObject $options The options for the query
* @return void
Copy link
Owner

Choose a reason for hiding this comment

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

You changed the return type but didn't update the doc block.

@@ -80,7 +81,7 @@ public function __construct(array $properties = [], array $options = [])
* @link http://book.cakephp.org/3.0/en/orm/entities.html#accessors-mutators
* @return string
*/
protected function _getFullPath(): string
protected function _getFullPath()
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the return type?

@@ -91,7 +92,7 @@ protected function _getFullPath(): string
* @link http://book.cakephp.org/3.0/en/orm/entities.html#accessors-mutators
* @return string
*/
protected function _getUrl(): string
protected function _getUrl()
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the return type?

@@ -117,7 +118,7 @@ public function path(array $options = []): string
* @param array $options Path options.
* @return string
*/
public function url(array $options = []): string
public function url(array $options = [])
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the return type?

@@ -102,7 +103,7 @@ protected function _getUrl(): string
* @param array $options Path options.
* @return string
*/
public function path(array $options = []): string
public function path(array $options = [])
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the return type?

@@ -130,7 +131,7 @@ public function url(array $options = []): string
* @param array $options Path options.
* @return string
*/
protected function _path(array $options): string
protected function _path(array $options)
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the return type?

@burzum
Copy link
Owner

burzum commented Feb 21, 2020

@skie I've made a few comments and asked some questions. Can we discuss them so I can resolve the PR please?

@burzum
Copy link
Owner

burzum commented Mar 14, 2020

@skie any feedback on this?

@skie
Copy link
Contributor Author

skie commented Jun 20, 2020

I would merge this PR as is. The types removed was autogenerated by external tool.

@burzum
Copy link
Owner

burzum commented Jun 25, 2020

@skie what tool? It sucks then. Use phpcs, phpstan and grumphp.

@skie
Copy link
Contributor Author

skie commented Jun 25, 2020

@burzum It is rector. It works perfect if all docblock 100% correct and follow modern trends.

@burzum
Copy link
Owner

burzum commented Jun 25, 2020

@skie but removing the types here is plain wrong?

@skie
Copy link
Contributor Author

skie commented Jun 25, 2020

I remember i had usecase which require this. I can remember what project it was but yes it was needed.

@burzum
Copy link
Owner

burzum commented Jun 25, 2020

@skie absolutely not. The only reason this is needed is an ancient php version that is > 7.0. The types are needed to merge this.

@burzum
Copy link
Owner

burzum commented Jul 14, 2020

Because of the author of this PR is not going to answer anymore I'm going to close this. This PR is going to be obsolete anyway in the near future when #208 is done.

@burzum burzum closed this Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants