Now MetaDataFilter takess also regexp. For example whern you want to #507

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

catalinux commented Nov 6, 2012

extract metadata if you would filter like this: --filter="Article"
would extract also for "ArticleItems" (article_items table). Now you
can use --filter="Article$" if you want only that table (articl)

@invalid-email-address @invalid-email-address invalid-email-address Now MetaDataFilter takess also regexp. For example whern you want to
extract metadata if you would filter like this: --filter="Article"
would extract also for "ArticleItems" (article_items table). Now you
can use --filter="Article$" if you want only that table (articl)
9b8db62

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-2128

Member

stof commented Nov 6, 2012

This does not follow the coding standards.

@henrikbjorn henrikbjorn and 1 other commented on an outdated diff Nov 6, 2012

lib/Doctrine/ORM/Tools/Console/MetadataFilter.php
@@ -66,9 +66,7 @@ public function accept()
$metadata = $it->current();
foreach ($this->filter as $filter) {
- if (strpos($metadata->name, $filter) !== false) {
- return true;
- }
+ if(preg_match("#$filter#",$metadata->name,$m)) return true;
@henrikbjorn

henrikbjorn Nov 6, 2012

Missing space after the if. Also you need to add { around the statement as defined in PSR-1/2

@stof

stof Nov 6, 2012

Member

and you need to escape the # in the filter as you use it as regex delimiter

@stof stof commented on an outdated diff Nov 6, 2012

lib/Doctrine/ORM/Tools/Console/MetadataFilter.php
@@ -51,7 +51,7 @@ static public function filter(array $metadatas, $filter)
public function __construct(\ArrayIterator $metadata, $filter)
{
- $this->filter = (array) $filter;
+ $this->filter = (array)$filter;
@stof

stof Nov 6, 2012

Member

Please keep this space

@stof stof and 3 others commented on an outdated diff Nov 6, 2012

lib/Doctrine/ORM/Tools/Console/MetadataFilter.php
@@ -66,7 +66,7 @@ public function accept()
$metadata = $it->current();
foreach ($this->filter as $filter) {
- if (strpos($metadata->name, $filter) !== false) {
+ if (preg_match("/$filter/", $metadata->name, $m)) {
@stof

stof Nov 6, 2012

Member

you don't need $m here as you don't use it.
And you still need to escape / in $filter. Changing the regex deleimiter does not remove the need to quote it. It only changes the character that needs quoting

@Majkl578

Majkl578 Nov 6, 2012

Contributor

I think an error in the regexp and no match should be differentiated, e.g. by checking for !== FALSE and !== 0 respectively.

@catalinux

catalinux Nov 6, 2012

Contributor

Should I throw an exception when an error in the regexp occurs?

@henrikbjorn

henrikbjorn Nov 6, 2012

$m should be $matches $m is an unclear name as it is not possible to know the intent without reading more of the code.

@catalinux

catalinux Nov 6, 2012

Contributor

In ConvertDoctrine1Schema.php we have the following code:

if (preg_match("/(\w+)\sas\s(\w+)/i", $column['name'], $matches)) {
            $name = $matches[1];
            $column['name'] = $name;
            $column['alias'] = $matches[2];
        }

No check of 0 vs FALSE for preg_match' result and escape. Can you show me some example.

I think is good ideea to remove $m. It polutes with no benefits. Otherwise I should have named it $matches.

@Majkl578

Majkl578 Nov 6, 2012

Contributor

Actually the problem with preg_* is a bit more complex. The errors might or might not produce an error message (severity of E_*), but if it doesn't there still might be a runtime error detectable by preg_last_error. You might want to look at this implementation.

@Majkl578 Majkl578 commented on the diff Nov 6, 2012

lib/Doctrine/ORM/Tools/Console/MetadataFilter.php
@@ -66,7 +66,16 @@ public function accept()
$metadata = $it->current();
foreach ($this->filter as $filter) {
- if (strpos($metadata->name, $filter) !== false) {
+ $pregResult = preg_match("/" . preg_quote($filter) . "/", $metadata->name);
@Majkl578

Majkl578 Nov 6, 2012

Contributor

You cannot use preg_qoute here, if you do so, $filter will not be treated as a regular expression (which was the intention of this pull request). And even if you do so, you still do not escape the delimiter, you would have to do this: preg_quote($filter, '/').

@catalinux

catalinux Nov 6, 2012

Contributor

This is true. I realized that after I pushed the code. Some unit test might be helpfull here

@beberlei

beberlei Nov 12, 2012

Owner

Just remove the preg_quote()

@stof stof commented on the diff Nov 6, 2012

lib/Doctrine/ORM/Tools/Console/MetadataFilter.php
@@ -66,7 +66,16 @@ public function accept()
$metadata = $it->current();
foreach ($this->filter as $filter) {
- if (strpos($metadata->name, $filter) !== false) {
+ $pregResult = preg_match("/" . preg_quote($filter) . "/", $metadata->name);
+ if ($pregResult === false) {
+ return false;
+ }
+
+ if ($pregResult === 0) {
+ return false;
@stof

stof Nov 6, 2012

Member

if you return false for both false and 0, there is no need to handle them separately.

@catalinux

catalinux Nov 6, 2012

Contributor

I know. There was one sugestiin that it should be treated diferently. My open question was: should I twrow an exception when error occurs?

@beberlei

beberlei Nov 12, 2012

Owner

Yes, please, Exception makes sense

Owner

beberlei commented Jan 2, 2014

Merged into master.

beberlei closed this Jan 2, 2014

@guilliamxavier guilliamxavier added a commit to guilliamxavier/doctrine2 that referenced this pull request Mar 20, 2016

@guilliamxavier guilliamxavier Add regex tests for MetadataFilter (PR #507) c7279c3

@guilliamxavier guilliamxavier added a commit to guilliamxavier/doctrine2 that referenced this pull request Jun 25, 2016

@guilliamxavier guilliamxavier Add regex tests for MetadataFilter (PR #507) 547ffed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment