refactor: phpcs and composer related updates #21
Conversation
|
Thanks for taking the time to improve it! We don't mind the size of the PR. This seems to be all internal changes correct? Assuming there are no external changes then i'd consider it a simple patch / cleanup. No need to break it up. |
src/CurrentPlugin.php
Outdated
| * @param int $size Size of the block to allocate in bytes. | ||
| */ | ||
| function allocate_block(int $size) : int | ||
| public function allocate_block(int $size): int |
There was a problem hiding this comment.
do these need to be public? These are fairly low-level methods that we don't want people calling in normal operation.
There was a problem hiding this comment.
Actually I am not completely sure what does these function behind the scene. As far as I checked it is used in HostFunctions.php. If you call these methods in the HostFunctions they should be public, if you don't you can set them as private.
There was a problem hiding this comment.
I've pushed some code and I changed the visibility of unused functions in CurrentPlugin.php. You can guide me about updating visibility of specific methods.
src/LibExtism.php
Outdated
| { | ||
| return $this->ffi->extism_current_plugin_memory($plugin); | ||
| } | ||
|
|
||
| function extism_current_plugin_memory_free(FFI\CData $plugin, FFI\CData $ptr): void | ||
| public function extism_current_plugin_memory_free(FFI\CData $plugin, FFI\CData $ptr): void |
There was a problem hiding this comment.
does this expose the methods to the external users? This is an internal API.
There was a problem hiding this comment.
They were already public if you not defined them explicitly protected or private.
From php.net
Class methods may be defined as public, private, or protected.
Methods declared without any explicit visibility keyword are defined as public.
There was a problem hiding this comment.
Since this function called in CurrentPlugin, we can't make it private.
There was a problem hiding this comment.
okay, just making sure we expose as little API to the public as possible. Maybe the class should be less accessible? I don't know if there is an equivalent of package only. But I'll leave @mhmd-azeez to pick out any changes that are needed. Thanks so much for this cleanup!
There was a problem hiding this comment.
Yeah, it seems like there is no package level access modifier in php. As a middle ground, I moved LibExtism.php to \Extism\Internal namespace, which is a pattern used by some php libraries like Symfony
There was a problem hiding this comment.
Great job @yigitcukuren, thank you very much! Can you please go through LibExtism.php functions and try to mark as many of them private as possible?
Thank you! I have just updated the visibility of all the internal methods. |
There was a problem hiding this comment.
Thank you very much for your excellent work @yigitcukuren, I moved LibExtism to \Extism\Internal namespace to make it clear that this class is for internal use only. Let me know if you have any concerns or issues with that. Otherwise, I can go on and merge the PR
It is perfectly fine! Please go ahead 🙏 |
Hey there,
I know this looks huge, but as far as I can see, there are no standards in the PHP code. I thought I might add some standards to make the PHP SDK more collaborative for everyone. If you think these changes are too much, I wouldn't mind creating multiple PRs for a smoother transition.