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

Php::Array Utility Functions #143

Closed
wants to merge 1 commit into from
Closed

Php::Array Utility Functions #143

wants to merge 1 commit into from

Conversation

tugrul
Copy link

@tugrul tugrul commented Nov 18, 2014

  • Added Php::Array::push()
  • Added Php::Array::pop()
  • Added Php::Array::getKeys()

  * Added Php::Array::push()
  * Added Php::Array::pop()
  * Added Php::Array::getKeys()
@EmielBruijntjes
Copy link
Member

Hi tugrul,

Thank you for your pull request. However, we will not add it to the project.

To start: not all arrays are stored in Php::Array objects. It is very well possible to store arrays in base Php::Value objects as well. For that reason, the Php::Value base class already has many array methods (for example the [] array access operator). Therefore, it would have been better had you added the push() and pop() methods to the Php::Value class instead of the Php::Array class.

Secondly, your code has a different coding style than the rest of the project. You can easily spot the differences in code style, and we do not want to turn this project into a mess.

But the real reason why this pull request is rejected, is because the PHP-CPP library tries to stay as close to PHP as possible. The Php::Value class (and also its derived Php::Array class) is meant to mimic a regular PHP $variable as much as possible. In PHP there is no construct like $variable.push("element"), but array_push($variable, "something") is used instead. We follow PHP in this. To push an item to an array with PHP-CPP, this is the code that you should be using:

Php::Array myArray;
Php::array_push(myArray, "something");

I'm not sure if the above example already works. If it doen't, a pull request would be very welcome.

Further more, in PHP that are hundreds if not thousands of different function that all work on $variables. array_push(), array_pop() and array_keys() are by far not the only ones. If we have to turn this long list of functions into methods on the Php::Value class, that class would become way too large to be maintainable (and I already find it too complicated now).

@tugrul
Copy link
Author

tugrul commented Nov 20, 2014

So I'm confused about Php::Array::getKeys method. I saw @andot 's implementation lately #115. Also it could be done with Php::HashIterator. In aside, looks like strange to me that thing to implement every method on Php::Value class. There are lot of usage isArray(), isObject(), isString() and so on to resolve polymorphic states. It could be better to seperate methods to related classes using virtual methods via polimorphism. Also should be implemented Php::Object::getProperties to iterate and return properties. Also should be implemented Php::Value::getKeys() and Php::Value::getProperties() placeholder methods that throws exception like "value is not array" or "value is not object".

I looked up the Php::array_push() function's implementation. It looks like call as user space function. This is pretty solution to avoid affect of PHP's codebase changes but has got low performance because it looks up CG(function_table) on every call. Utility functions -so others like array_shift, array_unshift, array_* - could be effective on produce user's BL module implementations, in other case zend_hash_* usage mess.

@EmielBruijntjes
Copy link
Member

The point is, the Php::Value class represents a PHP $variable. And because a $variable can be anything: a scalar, an object, an array, a string or even a callable, it also means that the Php::Value class must support the features of all these types.

In the majority of the situations, when you want to iterate over an array, the array comes from user space and is therefore stored in a Php::Value object, and not in a Php::Array. After all, if it was already stored in a Php::Array, it would mean that the array was created from C++ space, and it would be strange to iterate over it, as you have created it yourself, and there are more efficient data structures for storing arrays in C++ than in an instance of the Php::Value or Php::Array class.

So, if there is one place where the keys() methods should be implemented (and remember: we're not going to do that) it is in the Php::Value class, next to the operator[] implementation.

But because of the fact that PHP has hundreds of methods and we don't want to the Php::Value class to become a god-class, we use exactly the same approach that PHP uses: functions to retrieve properties of values: Php::array_keys(), Php::array_push(), et cetera.

Yes, you are right that the Php::array_push() function currently has indeed a very simple and not very efficient implementation. But nothing forbids you from sending in a pull request with a faster implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants