ValueGenerator constant detection #4238

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

3axap4eHko commented Apr 16, 2013

For example, we need to get a file like this:

<?php
return array(
    'module' => array(
        'moduleDir' => __DIR__ . '/../module'
    )
);

but we cant pass argument __DIR__, because this constant will be parsed and replaced to value, and we must quote it 'DIR' . And finally, after generation, we will not get this:

        'moduleDir' => __DIR__ . '/../module'

generated value will be like it:

        'moduleDir' => '__DIR__ . \'/../module\''

Changes:
Add ability to define environment constants by method

public function initEnvironmentConstants();

Also add ability to define custom constants by methods

public function addConstant($constant);
public function deleteConstant($constant)

property $constants must be type of ArrayObject for memory saving when pass it as argument for recursive generation by reference.

@weierophinney weierophinney commented on the diff Apr 16, 2013

library/Zend/Code/Generator/ValueGenerator.php
@@ -57,13 +59,19 @@ class ValueGenerator extends AbstractGenerator
* @var array
*/
protected $allowedTypes = null;
+ /**
+ * Autodetectable constants
+ * @var ArrayObject
@weierophinney

weierophinney Apr 16, 2013

Owner

Why are you using an ArrayObject and not simply an array?

Owner

weierophinney commented Apr 16, 2013

This looks good. My only comment is that I see no compelling reason for using ArrayObject to store the constants; in fact, based on the usage within the class, using array would simplify the code and likely make it more performant as well (fewer ArrayObject <-> array conversions).

Contributor

3axap4eHko commented Apr 18, 2013

property $constants must be type of ArrayObject for memory saving when pass it as argument for recursive generation by reference. If we have an array instead of ArrayObject, at each recursive call and initialized EnvironmentConstants we will create an array of about 1500 lines and it will take much memory for nested values.

Contributor

prolic commented Apr 20, 2013

performance comparision scripts?

@weierophinney weierophinney commented on the diff Apr 21, 2013

library/Zend/Code/Generator/ValueGenerator.php
+ *
+ * @param string $constant
+ *
+ * @return bool
+ */
+ public function deleteConstant($constant)
+ {
+ if (($index = array_search($constant, $this->constants->getArrayCopy())) !== false) {
+ $this->constants->offsetUnset($index);
+ }
+
+ return $index !== false;
+ }
+
+ /**
+ * Return constnat list
@weierophinney

weierophinney Apr 21, 2013

Owner

s/constnat/constant/

@weierophinney weierophinney added a commit that referenced this pull request Apr 21, 2013

@weierophinney weierophinney [#4238] Fix typo 218aa53

@weierophinney weierophinney added a commit that referenced this pull request Apr 21, 2013

@weierophinney weierophinney Merge branch 'feature/4238' into develop
Close #4238
8ce86dd
Owner

weierophinney commented Apr 21, 2013

Merged to develop for release with 2.2.0.

Maks3w referenced this pull request in zendframework/zend-code Nov 16, 2015

Closed

Require zend-stdlib #22

@Maks3w Maks3w commented on the diff Nov 16, 2015

library/Zend/Code/Generator/ValueGenerator.php
@@ -239,7 +318,7 @@ public function generate()
);
foreach ($rii as $curKey => $curValue) {
if (!$curValue instanceof ValueGenerator) {
- $curValue = new self($curValue);
+ $curValue = new self($curValue, self::TYPE_AUTO, self::OUTPUT_MULTIPLE_LINE, $this->getConstants());
@Maks3w

Maks3w Nov 16, 2015

Member

@3axap4eHko Did you use ArrayObject for this line?

I don't understand the performance improvements you claim. Non return by reference methods implemented on StdLib\ArrayObject are invoked.

The only thing I see the constants are shared by the instances created in this line, so the only need I see $this->constants must be provided as reference. Alternative solutions may

  • Use PHP native ArrayObject
  • Refactor getConstants for return by reference.
@3axap4eHko

3axap4eHko Nov 16, 2015

Contributor

Constants is a very big array, containing about 1k string values. Let's length of every string length is 10, therefore total size of array in memory would be greater then 10KB for one instance of ValueGenerator, that means for 1 property of generated class. So for 100 classes with 100 properties total size of constant arrays would be 10MB instead of 10KB for reference value

@3axap4eHko

3axap4eHko Nov 16, 2015

Contributor

Also you should keep in mind of allocating memory for every array in method. That 100 classes with 100 properties would be have a 10000 arrays

@Maks3w

Maks3w Nov 16, 2015

Member

So native ArrayObject is suitable for this case and Stdlib version is not required, isn't it?

@3axap4eHko

3axap4eHko Nov 16, 2015

Contributor

Stdlib\ArrayObject is a wrapper for backward version capability and nothing more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment