Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

DDC-2390: Remove Query dependency in SqlWalker and Parser #643

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Owner

beberlei commented Apr 4, 2013

To prevent future problems with illegal Query parameter access and also to decouple the namespaces by removing bidirectional dependency.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2391

@Majkl578 Majkl578 commented on the diff Apr 4, 2013

lib/Doctrine/ORM/Query/MetadataBag.php
+ * Returns NULL if {@link setMaxResults} was not applied to this query.
+ *
+ * @return integer Maximum number of results.
+ */
+ public function getMaxResults()
+ {
+ return $this->maxResults;
+ }
+
+ public function clearHints()
+ {
+ $this->hints = array();
+ }
+
+ /**
+ * Generate a string representation of that can be used as a cache key.
@Majkl578

Majkl578 Apr 4, 2013

Member

Why should this class be responsible for generating cache key?

@beberlei

beberlei Apr 4, 2013

Owner

As you see this code is moved from the Query class, which shouldnt be responsible for generating the cache key itself. It is a better target for this logic, because it encapsulates the state. Much better than Query

@Majkl578 Majkl578 commented on the diff Apr 4, 2013

lib/Doctrine/ORM/Query/MetadataBag.php
+ return $this->maxResults;
+ }
+
+ public function clearHints()
+ {
+ $this->hints = array();
+ }
+
+ /**
+ * Generate a string representation of that can be used as a cache key.
+ *
+ * @return string
+ */
+ public function __toString()
+ {
+ ksort($this->hints);
@Majkl578

Majkl578 Apr 4, 2013

Member

This (undesirably) changes the original array.

@beberlei

beberlei Apr 4, 2013

Owner

The sorting of the array is completely irrelevant, it has been in Query before and never required a sorting.

@Majkl578 Majkl578 commented on the diff Apr 4, 2013

lib/Doctrine/ORM/Query/MetadataBag.php
+ $this->hints = array();
+ }
+
+ /**
+ * Generate a string representation of that can be used as a cache key.
+ *
+ * @return string
+ */
+ public function __toString()
+ {
+ ksort($this->hints);
+
+ return md5(
+ var_export($this->hints, true) .
+ '&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults .
+ '&hydrationMode='.$this->hydrationMode.'DOCTRINE_QUERY_CACHE_SALT'
@Majkl578

Majkl578 Apr 4, 2013

Member

Typo - missing spaces around dots?

@FabioBatSilva FabioBatSilva commented on the diff Apr 4, 2013

lib/Doctrine/ORM/Query/SqlWalker.php
@@ -170,13 +170,13 @@ class SqlWalker implements TreeWalker
/**
* {@inheritDoc}
*/
- public function __construct($query, $parserResult, array $queryComponents)
+ public function __construct($metadata, $entityManager, $parserResult, array $queryComponents)
@FabioBatSilva

FabioBatSilva Apr 4, 2013

Owner

Could you please add typehints here ?

@Majkl578

Majkl578 Apr 4, 2013

Member

And maybe to $parserResult as well?

@beberlei

beberlei Apr 4, 2013

Owner

@FabioBatSilva this is from the interface TreeWalker, it never had typehints before. We could add them now, but that break the BC even more ;-)

@stof

stof Apr 5, 2013

Member

Well, the new arguments could be typehinted without breaking BC much than the current change IMO.

@FabioBatSilva FabioBatSilva commented on the diff Apr 4, 2013

lib/Doctrine/ORM/Query/TreeWalkerChain.php
{
- $this->_query = $query;
- $this->_parserResult = $parserResult;
- $this->_queryComponents = $queryComponents;
+ $this->metadata = $metadata;
@FabioBatSilva

FabioBatSilva Apr 4, 2013

Owner

Please align =

@stof stof commented on the diff Apr 5, 2013

lib/Doctrine/ORM/Query/TreeWalker.php
@@ -34,7 +34,7 @@
* @param \Doctrine\ORM\Query\ParserResult $parserResult The result of the parsing process.
@stof

stof Apr 5, 2013

Member

you need to update the phpdoc

@stof stof commented on the diff Apr 5, 2013

lib/Doctrine/ORM/Query/TreeWalkerAdapter.php
@@ -50,13 +50,19 @@
private $_queryComponents;
@stof

stof Apr 5, 2013

Member

couldn't you rename these private properties to remove the leading underscore ? this is regularly done by @guilhermeblanco in classes where changes are applied, and it is not a BC break for private properties

@stof

stof Apr 5, 2013

Member

Well, actually, it is also done by you in the TreeWalkerChain :)

Owner

beberlei commented Apr 6, 2013

I will update the PR with styling fixes, when we found a decision if this is actually worth merging or not.

Owner

beberlei commented May 1, 2013

This is not necessary, we just have to be more careful with changes introduced in the SqlWalker

@beberlei beberlei closed this May 1, 2013

@Ocramius Ocramius deleted the DDC-2390 branch Jul 7, 2016

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