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

Add optional param domain to Gettext::query() #12606

Closed
wants to merge 1 commit into from
Closed

Add optional param domain to Gettext::query() #12606

wants to merge 1 commit into from

Conversation

mervick
Copy link
Contributor

@mervick mervick commented Feb 9, 2017

Hello!

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change:
Fix #12598
Ref #12600

Thanks

Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

Declaration of Phalcon\Translate\Adapter\Gettext::query must be compatible with Phalcon\Translate\AdapterInterface::query

@mervick
Copy link
Contributor Author

mervick commented Feb 12, 2017

@sergeyklay how about to create some custom method for this? for example

public function dquery(string! domain, string! index, placeholders = null) -> string

or

public function dquery(string! index, placeholders = null, string! domain = null) -> string

@sergeyklay
Copy link
Contributor

@mervick Right now I'll close this issue and mark it as "Won't fix". Also I plan to clean Gettext::query to remove redundant and not worked domain-related stuff.

I like you suggestion. But we accept only bug fixes for 3.0.x. Could you please open new feature request in order to link it in next PRs to the 3.1.x?

Thanks

@sergeyklay sergeyklay added the wontfix The issue will not be fixed or implemented label Feb 12, 2017
@sergeyklay sergeyklay closed this Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix The issue will not be fixed or implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants