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

Performance merged #191

Merged
merged 20 commits into from Jan 23, 2013
Merged

Conversation

@patrickallaert
Copy link
Contributor

patrickallaert commented Dec 13, 2012

Pull request merging the changes made by Kore in performance branch.

@ezrobot

This comment has been minimized.

Copy link
Contributor

ezrobot commented Dec 13, 2012

This Pull Request does not respect our Coding Standards, please, see the report below:

FILE: .../Publish/Core/Persistence/Legacy/Content/Search/Gateway/EzcDatabase.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 208 | ERROR | A cast statement must not be followed by a space
--------------------------------------------------------------------------------


FILE: ...Z/Publish/Core/Persistence/Legacy/Content/Type/MemoryCachingHandler.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  11 | ERROR | The "use" keyword must be used to import one class/namespace at
     |       | a time.
  11 | ERROR | The "use" keyword must not be spread over more than one line.
 317 | ERROR | File must not contain multiple empty lines in a row; found 2
     |       | empty lines
--------------------------------------------------------------------------------

@bdunogier
bdunogier reviewed Dec 13, 2012
View changes
eZ/Publish/Core/Persistence/Legacy/Content/Type/MemoryCachingHandler.php Outdated
@@ -0,0 +1,421 @@
<?php
/**
* File containing the Content Type Handler class

This comment has been minimized.

Copy link
@bdunogier

bdunogier Dec 13, 2012

Member

Wrong :-)

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Dec 13, 2012

Author Contributor

Damn, I forgot creating a Sniff for checking that ;-)

}

/**
* Adds a new field definition to an existing Type.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Dec 13, 2012

Member

Shouldn't we, for all of those, use {@inheritdoc} instead ?

This comment has been minimized.

Copy link
@andrerom

andrerom Dec 13, 2012

Member

we probably should, would save us lots of unaligned doc.

This comment has been minimized.

Copy link
@patrickallaert

patrickallaert Dec 13, 2012

Author Contributor

The thing is that documentation tools behaves differently. Some supports inheritance with a dedicated flag, some inherit by default,... We should probably decide what to do based on some research in this regard.

I am personally in favour of not documenting anything if there is no doc change, and use {@inheritdoc} in the case we have something that is changed and use it for copy/paste parent doc. Like in:

/**
 * Wrapper for in-memory cache. {@inheritdoc}
 */

The following shouldn't be used IMHO as it should be documentation tool's default behaviour:

/**
 * {@inheritdoc}
 */

This comment has been minimized.

Copy link
@gggeek

gggeek Dec 13, 2012

Contributor

-1 for "standalone" {@inheritdoc} tags. Line noise (as in: makes reading the actual code harder, and for the generated doc, well, let's improve doc generators instead)

This comment has been minimized.

Copy link
@bdunogier

bdunogier Dec 14, 2012

Member

The fact that @inheritdoc reduces readability is an argument. But honestly,
I'd rather have to jump to ther parent method, rather than having outdated
documentation due to copy/paste + entropy.

I think that we can require that our users, at least those who have
interest in reading our code will rely either on the generated
documentation OR on a feature-rich IDE rather than on reading code only.
This assumption makes it really easier to provide, accurate, up-to-date
documentation.

In any case, I think we at least all agree that if we choose not to
copy/paste parent documentation when it is identical, it is useless to add
@inheritdoc. This should be automatically handled by IDEs and doc
generators. And actually, it is.

I have checked support on sami (http://ezsystems.github.com/ezp-next/sami),
and it indeed isn't perfect. I have also checked PhpStorm 5. There are
the results with a validate function, in the Image\Type class, inheriting
from FieldType.

No phpdoc at all:

  • sami: documentation from parent is re-used
  • phpstorm: documentation from parent is re-used

Inline {@inheritdoc} with extra documentation
/**
* Overridden documentation.
* {@inheritdoc}
*/

  • sami: Prints out {@inheritdoc}
  • phpstorm: Doesn't take the local changes into account

Override short description in the child method

  • sami: replaces the parent's short description with the child's. Parameter
    & return & @throws docs are maintained.
  • phpdoc: replaces the parent's short description with the child's.
    Parameter & return & @throws docs are maintained.
@ezrobot

This comment has been minimized.

Copy link
Contributor

ezrobot commented Dec 13, 2012

This Pull Request does not respect our Coding Standards, please, see the report below:

FILE: ...workspace/eZ/Publish/API/Repository/Tests/Stubs/URLAliasServiceStub.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 368 | ERROR | Public method name
     |       | "URLAliasServiceStub::_removeAliasesForLocation" is not in camel
     |       | caps format
--------------------------------------------------------------------------------


FILE: .../eZ/Publish/Core/Persistence/Legacy/Tests/Content/SearchHandlerTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 23 | ERROR | File must not contain multiple empty lines in a row; found 2
    |       | empty lines
--------------------------------------------------------------------------------

kore added 10 commits Nov 26, 2012
Should be sane, even limited compared to what zetacomponents offer. But
with the limited set of table and column names, this should be fast
enough.
Only requests in one single request.
Especially mind: We need to clear internal caches during rollback.
Made code readable again and removed unused bits
@ezrobot

This comment has been minimized.

Copy link
Contributor

ezrobot commented Dec 14, 2012

This Pull Request does not respect our Coding Standards, please, see the report below:

FILE: .../eZ/Publish/Core/Persistence/Legacy/Tests/Content/SearchHandlerTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 23 | ERROR | File must not contain multiple empty lines in a row; found 2
    |       | empty lines
--------------------------------------------------------------------------------

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Dec 17, 2012

Tested on a real life website and those patches make a big difference :-) Still not perfect, but way better.

@patrickallaert

This comment has been minimized.

Copy link
Contributor Author

patrickallaert commented Dec 17, 2012

All unit and integration tests are now passing.
All CS issues are solved as well.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Jan 23, 2013

Only thing would be that I think "eZ/Publish/Core/Persistence/Legacy/Content/Type/MemoryCachingHandler.php" is probably unneeded when SPI cache is introduced. But given this is pr instance it does not hurt, and refactoring needed to de couple handlers to be able to inject outer spi persistence cache handler is to high. So: +1 as is

@patrickallaert

This comment has been minimized.

Copy link
Contributor Author

patrickallaert commented Jan 23, 2013

Thanks for your comment @andrerom, I second your opinion.

patrickallaert added a commit that referenced this pull request Jan 23, 2013
Performance merged
@patrickallaert patrickallaert merged commit 2880245 into ezsystems:master Jan 23, 2013
1 check failed
1 check failed
default The Travis build failed
Details
@patrickallaert patrickallaert deleted the patrickallaert:performanceMerged branch Jan 23, 2013
@gggeek

This comment has been minimized.

BUG - you need an array_unique here, or different SQL, as you can get back duplicate object ids

This comment has been minimized.

Copy link
Contributor

gggeek replied Jun 4, 2013

OPTIMIZATION: if there is no offset and limit set, just execute 1 query instead of 2, and do a php count() on the resulting array. It makes db happier

This comment has been minimized.

Copy link
Member

andrerom replied Jun 4, 2013

Use issue tracker please.

This comment has been minimized.

Copy link
Contributor

lolautruche replied Nov 7, 2013

Ref EZP-21906

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
You can’t perform that action at this time.