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

Pluralization class replaced with Doctrine/Inflector #392

Closed
wants to merge 8 commits into from
Closed

Pluralization class replaced with Doctrine/Inflector #392

wants to merge 8 commits into from

Conversation

Markkaz
Copy link

@Markkaz Markkaz commented Mar 10, 2013

Replacing the Pluralization class reduces maintenance our own solution and makes it easier to configure.

@lsmith77
Copy link
Member

have a look at the diff. there are quite a few issues with intending. the name of the inflector service should maybe be configurable under the routing config.

@Markkaz
Copy link
Author

Markkaz commented Mar 10, 2013

Yes, I was not sure where to put it, in Util or under routing.
In the end I put it in the Util, because previously it was there already. Maybe it is better under routing indeed, because that's where it is used.

What do you mean with issues with indending? Or was this about the place where I put the configuration and classes?
Maybe I can learn something from it :-)

@lsmith77
Copy link
Member

I wasnt talking about the namespace. i was talking about adding the name of the service into the Configuration.php to allow users to easily replace the default one with a customized one.

As for indenting, just look at the diff, you will see that in several places your additional code isnt aligned the same way as the existing code.

@Markkaz
Copy link
Author

Markkaz commented Mar 10, 2013

Aah, now I understand. Sorry, that is indeed a good suggestion. Making the name of the Inflector class configurable. I will change that one.

On the second issue, I read intending instead of indenting, but see that there are indenting issues in the diff. Strange because I don't have them on my machine. Let's see if I can get those fixed too :-)

@lsmith77
Copy link
Member

for the inflector i would make the service configurable .. not sure how useful it is to configure just the class.

@Markkaz
Copy link
Author

Markkaz commented Mar 10, 2013

Sorry, that was what I meant. Busy finding out how to do it :-)

@Markkaz
Copy link
Author

Markkaz commented Mar 10, 2013

Configuration added and indention issues fixed.

@lsmith77
Copy link
Member

looks good .. can you adjust the travis.yml file as follows:

before_script:
  - composer require symfony/framework-bundle:${SYMFONY_VERSION} --dev

that should fix the tests.

@Markkaz
Copy link
Author

Markkaz commented Mar 11, 2013

And done

@lsmith77
Copy link
Member

ok thx. i just send out a request for people to test this on twitter:
https://twitter.com/lsmith/status/311770393113415682

@lsmith77
Copy link
Member

@Elexy could you test this PR with your apps?

@asm89
Copy link
Member

asm89 commented Mar 13, 2013

@lsmith77 @Markkaz Maybe also squash the commits in this PR? I'll try to find some time to run one of our apps against this PR, but that app hasn't been touched in quite a while, so it will depend on how much time I must spend on upgrading.

@lsmith77
Copy link
Member

this PR now need a rebase.

@@ -10,7 +10,7 @@ env:
# - SYMFONY_VERSION=dev-master

before_script:
- composer require symfony/framework-bundle:${SYMFONY_VERSION}
- composer require symfony/framework-bundle:${SYMFONY_VERSION} --dev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this is really needed or what is the advantage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer needed once the PR is rebased to master

@Elexy
Copy link

Elexy commented Mar 18, 2013

All my apps tests run successfully on the 0.11 branch. 0.12 gives unrelated errors by itself.

@lsmith77
Copy link
Member

just to clarify. what so you mean with 0.11 branch? the code in this branch (which is based on the 0.11 branch) ?

@Elexy
Copy link

Elexy commented Mar 18, 2013

Sorry; when I specify "friendsofsymfony/rest-bundle": "0.12.*@dev" I get serializer errors. with or without PR. So it's clearly unrelated. I will investigate separately and look into c334e61

When I leave it at stability 0.11 and apply pull the PR all my functional (REST API) tests pass.

@lsmith77
Copy link
Member

OK, thanks. @asm89 did you have a chance to test?

@Elexy
Copy link

Elexy commented Mar 19, 2013

Just tested with SF2.2.0 + this @0.12 (dev-master) and after a rm app/cache/* -rf all tests pass as well. app/console cache:clear -e test didn't do the trick.

My previous test was with SF2.1..

@lsmith77 lsmith77 mentioned this pull request Mar 19, 2013
@lsmith77
Copy link
Member

see #403

@lsmith77
Copy link
Member

i have merged #403

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.

None yet

5 participants