Skip to content

[2.1] Gettext support #634

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

Merged
merged 276 commits into from
Sep 29, 2011
Merged

[2.1] Gettext support #634

merged 276 commits into from
Sep 29, 2011

Conversation

xaav
Copy link
Contributor

@xaav xaav commented Apr 23, 2011

Gettext is the web translation standard today, so I figured I might include it in Symfony2.

Note: some of this code is under the BSD license, in case that matters. I did get it from somewhere else.

I haven't debugged this either, but you may be able to find errors.

@@ -1,3 +1,6 @@
phpunit.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll revert them as soon as I get access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, you can revert these changes after you merge this branch. I don't want my IDE files in the repository!

Copy link

Choose a reason for hiding this comment

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

Simply add a global .gitignore file to your configuration and add your IDE files there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay! The gitignore is reverted, and no IDE files in the repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

What i always do is modify the .git/info/exclude file, which works as .gitignore but is local to the clone/checkout. that way you dont have to change a projects .gitignore file.

@lsmith77
Copy link
Contributor

@xaav
Copy link
Contributor Author

xaav commented Apr 24, 2011

It appears that I need to refractor the loader a bit to make it compatible with symfony.

@xaav
Copy link
Contributor Author

xaav commented Apr 24, 2011

Okay; I've made a quick fix, so it should work now; however I still need to test this and see if it actually works.

Also, I would like to clear out some unnecessary code (performance reasons) before merging this branch.

@xaav
Copy link
Contributor Author

xaav commented Apr 28, 2011

Anyone care to make any comments on this pull request?

I am still working on the code, and I'll push it up when I'm done.

@xaav
Copy link
Contributor Author

xaav commented Apr 29, 2011

Ok, I've tested this and the PoFileLoader works.

@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

These Eclipse files should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh... the changes to the .gitignore were suppossed to prevent this.

@xaav
Copy link
Contributor Author

xaav commented May 1, 2011

Okay, everything appears to be working properly; I've tested this on several files.

I'm afraid if I fix the gitignore, my IDE files will end up in the repository.

I'm going to talk to the authors and see what they say.

@@ -11,6 +11,8 @@
<parameter key="translation.loader.php.class">Symfony\Component\Translation\Loader\PhpFileLoader</parameter>
<parameter key="translation.loader.yml.class">Symfony\Component\Translation\Loader\YamlFileLoader</parameter>
<parameter key="translation.loader.xliff.class">Symfony\Component\Translation\Loader\XliffFileLoader</parameter>
<parameter key="translation.loader.po.class">Symfony\Component\Translation\Loader\PoFileLoader</parameter>
<parameter key="translation.loader.mo.class">Symfony\Component\Translation\Loader\MoFileLoader</parameter>
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@xaav
Copy link
Contributor Author

xaav commented May 5, 2011

All issues have been fixed. Please merge this branch.

</container>
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a mess with the line-endings here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@xaav
Copy link
Contributor Author

xaav commented May 5, 2011

Fixed line endings. Anything else?

@xaav
Copy link
Contributor Author

xaav commented May 6, 2011

Ok, well then if there is nothing else, please merge this branch.

@lsmith77
Copy link
Contributor

lsmith77 commented May 6, 2011

are you really complying with the BSD license by only listing the author and not the location of the original source itself?

Lukas

On 29.04.2011, at 04:22, xaav reply@reply.github.com wrote:

Ok, I've tested this and the PoFileLoader works.

Reply to this email directly or view it on GitHub:
#634 (comment)

@stloyd
Copy link
Contributor

stloyd commented May 6, 2011

@xaav please read http://dev.lithify.me/lithium/source/libraries/lithium/LICENSE.txt and fix those files with that license info.

About merge, it will be NOT merged not as it's planned for version 2.1 (we just relased 2.0 beta1).

@xaav
Copy link
Contributor Author

xaav commented May 6, 2011

[nateabele]

As long as the copyright stays with the affected portions of the code, and a
pointer / URL back to the origin project (http://lithify.me), that's fine. Thanks for asking.

I'll fix the @author so it says copyright.

Commits
-------

c4a0f79 Updates according to suggestions.
6aec789 Added tests.
54454ba Added generic filtering to ParameterBag.

Discussion
----------

Added generic filtering to ParameterBag.

Adds filtering convenience using PHP's filter_var() e.g.

    $request->get->filter($key, '', false, FITLER_SANITIZE_STRING);

See http://php.net/manual/en/filter.filters.php for capabilities.

---------------------------------------------------------------------------

by GromNaN at 2011/09/25 15:41:50 -0700

What is the use case ?

---------------------------------------------------------------------------

by drak at 2011/09/25 15:52:19 -0700

Input variable validation/sanitization.  ParameterBag has a few built in like `getAlnum()` for example.  This method offer's PHP's full filtering and sanitization suite.

---------------------------------------------------------------------------

by fabpot at 2011/09/27 00:56:41 -0700

Can you add some unit tests for this new feature?

---------------------------------------------------------------------------

by drak at 2011/09/27 00:58:56 -0700

Sure thing.

---------------------------------------------------------------------------

by drak at 2011/09/27 01:07:03 -0700

Before I make the commit, is the method name ok for you or would you prefer it is called `getFiltered()`?

---------------------------------------------------------------------------

by fabpot at 2011/09/27 01:13:46 -0700

`filter` sounds good to me.

---------------------------------------------------------------------------

by drak at 2011/09/27 02:37:01 -0700

I've added some tests.

---------------------------------------------------------------------------

by stloyd at 2011/09/27 02:42:42 -0700

@Drak IMO you must check that user don't use unknown filter and/or flags for filter.

---------------------------------------------------------------------------

by drak at 2011/09/27 02:48:38 -0700

@stloyd - I'm not sure that's practical at all, this is a wrapper for a built-in PHP function and I don't understand why we would need validate arguments for a PHP function - it's the coder's job to use the API correctly - none of the inputs to this function are coming from a web request.  It would also mean that the API would need to keep track of any upstream changes to constants in the PHP engine (which are just integers after all).  It's really just not practical.

---------------------------------------------------------------------------

by stealth35 at 2011/09/27 05:16:50 -0700

@Drak it's could be cool to use `filter_id` ✌️

    if (is_string) {
        $filter = filter_id($filter);
    }

---------------------------------------------------------------------------

by drak at 2011/09/27 07:05:42 -0700

@stealth35 regarding this

    if (is_string) {
        $filter = filter_id($filter);
    }

I believe strongly in the use of IDEs when coding and autocomplete nicely provides when you type `FILTER_`.  Additionally, `filter_id()` only works on filters, but not for the flags, so I'm not entirely sure how useful it would be overall compared to using a good IDE (which you need when working with complex frameworks anyhow, imo :)

---------------------------------------------------------------------------

by drak at 2011/09/27 07:30:10 -0700

Ok check it now.
@xaav
Copy link
Contributor Author

xaav commented Sep 28, 2011

Actually, someone could just fork my branch and send their own PR.

@stealth35 If want gettext support, please do the above. I see no reason to leave this PR open as I'm the only one who has access to my repo.

Commits
-------

f7bf7b5 fixed condition
181332b added a Controller:getUser() shortcut to recover the current user

Discussion
----------

[2.1] added a Controller:getUser() shortcut to recover the current user
…ntent() and Crawler::addXmlContent() via libxml functions
@ghost
Copy link

ghost commented Sep 28, 2011

I had a go rebasing this but since this was not done as a topic branch it's so so simple - considering there are just two new files added and one modification to an XML file + tests, it would be better to just add the changes rather than try to squash about 20 commits stuck between merges.

@fabpot
Copy link
Member

fabpot commented Sep 28, 2011

@Drak: we should keep author names.

@stof
Copy link
Member

stof commented Sep 28, 2011

I rebased andd squashed it by keeping the authorship: https://github.com/stof/symfony/compare/gettext_loader

If someone wants to add the dumper, he can work from this branch. @fabpot should I send a new PR to merge the loader now or should I wait for someone adding the dumper ?

@ghost
Copy link

ghost commented Sep 28, 2011

@fabpot - I understand that but in this case it's really time consuming to try - lost an hour on it so far. I can tamper with the patches to make them look like they came from @xaav & @geoff but then just crediting in the commit message should also be an option? Look at https://github.com/drak/symfony/commits/gettext/ e.g. https://github.com/drak/symfony/commit/b690ba83b2096f744c2da06789ef6315b0a6dc0f
The other option is to tamper with patches and apply them that will retain the author, but in the case of Mo and Po reader files there are two authors - that's why I opted for credit in the commit messages.

@ghost
Copy link

ghost commented Sep 28, 2011

By the way, remaining work on this issue seems to be tests for the MO reader, maybe a dumper, and a quick review of CS. Btw, do you have a set of CS rules for NetBeans? The built-in code formatter can take care of stuff like this much easier - honestly looking at this ticket I think the authors just got fedup of small details when automation could have solved a lot of this (via code formatting in the IDE - Eclipse has it too). If it's OK we can move this to a new PR using my branch.

@stealth35
Copy link
Contributor

@stof I'll add the dumpers

@stof
Copy link
Member

stof commented Sep 28, 2011

arf, I lost part of the authorship when squashing as there is 2 authors. Missed that :(

@ghost
Copy link

ghost commented Sep 28, 2011

@stof - try this instead: squash Geoff's commits into one, then squash all xaav's into another, that way there is full authorship history and just two commits.

@stof
Copy link
Member

stof commented Sep 28, 2011

hmm, reading the comments in the PR, it sems like @xaav commented after changes done by Geoff. Is it simply a different email address owned by yourself @xaav ?

Commits
-------

dd20f01 Fixed assets:install to use a relative path instead of an absolute

Discussion
----------

[2.1] Fixed "assets:install" to create relative instead of absolute symlinks

This is a fairly simple fix so that the symlinks are relative to the resources rather than an absolute path that breaks from machine-to-machine or upon deployment.

We were trying to figure out why styles were messed up for other contributors, but then found that the paths were hard-coded for my machine :)

---------------------------------------------------------------------------

by ericclemmons at 2011/06/04 09:44:11 -0700

Any other thoughts/updates on this?

---------------------------------------------------------------------------

by fabpot at 2011/06/04 22:31:39 -0700

We have such a feature in symfony1 and IIRC it does not work very well. One problem is when you use symlink for some bundles. Then, you should not use a relative symlink as there is a common path between the two.

---------------------------------------------------------------------------

by ericclemmons at 2011/06/05 09:55:00 -0700

Sorry, I didn't think that we would be an issue since the Bundle "Best Practices" states to not include other bundles as dependencies.

If absolute links are a must, then the next alternative is for collaborators to add "/web/bundles" to .gitignore and each person run "assets:install" upon installation/update.

I was personally hoping there were a way to have this versioned for easier deployment.

On Jun 5, 2011, at 12:31 AM, fabpot<reply@reply.github.com> wrote:

> We have such a feature in symfony1 and IIRC it does not work very well. One problem is when you use symlink for some bundles. Then, you should not use a relative symlink as there is a common path between the two.
>
> --
> Reply to this email directly or view it on GitHub:
> symfony#1173 (comment)

---------------------------------------------------------------------------

by henrikbjorn at 2011/06/27 04:56:58 -0700

``` php
<?php
// ...
->addOption('relative', null, InputOption::VALUE_NONE, 'The --symlink option will generate relative paths')
// ...
```

and just default to absolute paths ?

---------------------------------------------------------------------------

by ericclemmons at 2011/06/27 08:37:50 -0700

I'm very supportive of that compromise.  Up to @fabpot if I should add this back in, since relative paths were apparently problematic with symfony1.

---------------------------------------------------------------------------

by sbusch at 2011/07/15 08:46:01 -0700

+1

I'm developing on Mac and the files are mounted on a Linux box which serves the project. The paths are not the same on those two systems. If I accidentally install assets on my Mac the absolute paths won't work on the Linux webserver.

Other scenario: one teammate could add those symlinks by accident to the git repository, which breaks all other installations.

Relative symlinks could help a lot in these cases.

---------------------------------------------------------------------------

by ericclemmons at 2011/07/15 08:47:53 -0700

@sbusch Your issues are the same as mine, which prompted this ticket :)

Until this gets @fabpot's blessing, it's best to simply add `web/bundles` to your `.gitignore` file and tell your users to always run `assets:install --symlink` each time they pull down code & something breaks ;)

---------------------------------------------------------------------------

by sbusch at 2011/07/15 08:58:33 -0700

The handling (calculation) of relative symlinks IMO fits better to the `symlink()` method of `\Symfony\Component\HttpKernel\Util\Filesystem`. Possible method signature:

symfony/src/Symfony/Component/HttpKernel/Util/Filesystem.php:

```php
<?php
// ...

    /**
     * Creates a symbolic link or copy a directory.
     *
     * @param string  $originDir     The origin directory path
     * @param string  $targetDir     The symbolic link name
     * @param Boolean $copyOnWindows Whether to copy files if on Windows
     * @param Boolean $makeRelative  Whether to try to create a relative link
     */
    public function symlink($originDir, $targetDir, $copyOnWindows = false, $makeRelative = false)
    {
```

And what about changing the `--symlink` option to optionally have a value, instead of adding a new depending option? E.g. `--symlink[=absolute|relative]`, with "absolute" as default:

symfony/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php:

```php
<?php
// ...

class AssetsInstallCommand extends ContainerAwareCommand
{
    /**
     * @see Command
     */
    protected function configure()
    {
        $this
            ->setDefinition(array(
                new InputArgument('target', InputArgument::REQUIRED, 'The target directory (usually "web")'),
            ))
            ->addOption('symlink', null, InputOption::VALUE_OPTIONAL, 'Symlinks the assets instead of copying it. Allowed values: "absolute" (default) and "relative".', 'absolute')
            ->setHelp(<<<EOT
The <info>assets:install</info> command installs bundle assets into a given
directory (e.g. the web directory).

<info>./app/console assets:install web [--symlink]</info>

A "bundles" directory will be created inside the target directory, and the
"Resources/public" directory of each bundle will be copied into it.

To create a symlink to each bundle instead of copying its assets, use the
<info>--symlink</info> option. Use <info>--symlink=relative</info> for relative symlinks.
EOT
            )
            ->setName('assets:install')
        ;
    }

    /**
     * @see Command
     *
     * @throws \InvalidArgumentException When the target directory does not exist
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        if (!is_dir($input->getArgument('target'))) {
            throw new \InvalidArgumentException(sprintf('The target directory "%s" does not exist.', $input->getArgument('target')));
        }

        if ($input->hasOption('symlink'))
        {
            if (!function_exists('symlink')) {
                throw new \InvalidArgumentException('The symlink() function is not available on your system. You need to install the assets without the --symlink option.');
            }
            if (!in_array($input->getOption('symlink'), array('absolute', 'relative'))) {
                throw new \InvalidArgumentException(sprintf('Invalid value "%s" for option "symlink"', $input->getOption('symlink')));
            }
        }

        $filesystem = $this->getContainer()->get('filesystem');

        // Create the bundles directory otherwise symlink will fail.
        $filesystem->mkdir($input->getArgument('target').'/bundles/', 0777);
        foreach ($this->getContainer()->get('kernel')->getBundles() as $bundle) {
            $originDir = $bundle->getPath().'/Resources/public';
            if (is_dir($originDir)) {
                $targetDir = $input->getArgument('target').'/bundles/'.preg_replace('/bundle$/', '', strtolower($bundle->getName()));
                $output->writeln(sprintf('Installing assets for <comment>%s</comment> into <comment>%s</comment>', $bundle->getNamespace(), $targetDir));

                $filesystem->remove($targetDir);

                if ($input->hasOption('symlink')) {
                    $filesystem->symlink($originDir, $targetDir, false, $input->getOption('symlink') == 'relative');
                } else {
                    $filesystem->mkdir($targetDir, 0777);
                    $filesystem->mirror($originDir, $targetDir);
                }
            }
        }
    }
```

---------------------------------------------------------------------------

by sbusch at 2011/07/15 09:04:46 -0700

@ericclemmons: yes, that's our current workaround. I started with manually converting absolute links to relative ones, but that quickly got very annoying ;-)

After that I tried to implement the generation of relative links by myself (where the proposals of my previous comment come from) until I found your PR.

---------------------------------------------------------------------------

by henrikbjorn at 2011/07/18 00:20:38 -0700

@sbush if it defaults to something how would you turn it off ?

---------------------------------------------------------------------------

by stof at 2011/07/18 00:26:16 -0700

@henrikbjorn a default value for the option is used when using ``--symlink`` without the value. If you don't use the option at all, it is disabled.

---------------------------------------------------------------------------

by stof at 2011/07/18 11:58:29 -0700

In fact no. The default value seems to be also used when the option is not set at all. @fabpot is this intended ?

---------------------------------------------------------------------------

by Seldaek at 2011/07/19 05:18:29 -0700

Symlinks on windows, although technically possible, don't quite work with PHP on most setups. Also git doesn't seem to support them either on windows (not sure why not). For those reasons, and although I'm sure this doesn't apply to every project, I would recommend you just have everyone run `assets:install [--symlink]` on their local machine, and make that command run on the server as part of your deployment process.

---------------------------------------------------------------------------

by ericclemmons at 2011/07/19 06:15:34 -0700

Nobody is even entertaining --relative?

On Jul 19, 2011, at 7:18 AM, Seldaek<reply@reply.github.com> wrote:

> Symlinks on windows, although technically possible, don't quite work with PHP on most setups. Also git doesn't seem to support them either on windows (not sure why not). For those reasons, and although I'm sure this doesn't apply to every project, I would recommend you just have everyone run `assets:install [--symlink]` on their local machine, and make that command run on the server as part of your deployment process.
>
> --
> Reply to this email directly or view it on GitHub:
> symfony#1173 (comment)

---------------------------------------------------------------------------

by Gregwar at 2011/08/10 08:56:27 -0700

I agree with the idea of proposing a --relative option, I'm currently working on a Samba mounted filesystem and I'm forced to create manually symlinks to get things working since the paths are not the same
Commits
-------

5146a1f [EventDispatcher] Added possibility for subscribers to subscribe several times for same event

Discussion
----------

[EventDispatcher] Added possibility for subscribers to subscribe several times for same event

[EventDispatcher] Added possibility for subscribers to subscribe several times for same event

closes symfony#2146

And it is of course fully BC :)

---------------------------------------------------------------------------

by jalliot at 2011/09/09 17:34:07 -0700

If merged, symfony#2021 will have to reflect the change too
* 2.0:
  fixed usage of LIBXML_COMPACT as it is not always available
  Fixed the phpdoc
@xaav
Copy link
Contributor Author

xaav commented Sep 28, 2011

No need to go out of the way to give me credit. You don't even need to mention me, if you want.

@stof
Copy link
Member

stof commented Sep 28, 2011

@stealth35 then simply use the rebased branch I did.

@fabpot fabpot merged commit e02915b into symfony:master Sep 29, 2011
fabpot added a commit that referenced this pull request Nov 9, 2011
Commits
-------

d974a4a Merge pull request #4 from stealth35/test_mo_loader
cf05646 delete useless tests
19f9de9 [Translation] fix gettext tests
965f2bf Merge pull request #3 from stealth35/test_mo_loader
9c2a26d [Translation] add Mo loader tests
9af2342 [Translation] Added the gettext loaders

Discussion
----------

[Translation] Added the gettext loaders

This is the squashed version of the work done by @xaav in #634.

@stealth35 you said you will work on the dumpers. do you have some stuff on it ?

---------------------------------------------------------------------------

by drak at 2011/10/24 19:28:43 -0700

Is there any more progress with this?

---------------------------------------------------------------------------

by stealth35 at 2011/10/25 00:57:19 -0700

I work on the dumpers, but the Po loader is wrong, caus' the Po ressource can be multiline,

     msgid ""
     "Here is an example of how one might continue a very long string\n"
     "for the common case the string represents multi-line output.\n"

http://www.gnu.org/software/gettext/manual/gettext.html#PO-Files

Anyway the Po format is an intermediate format to Mo file, (like .txt to .res file for ICU), IMO we can just support the real gettext format : Mo

---------------------------------------------------------------------------

by stealth35 at 2011/11/03 02:00:24 -0700

@stof The MO Dumper is ready (stealth35/symfony@f2d1d5b), should we keep the PO format ?

---------------------------------------------------------------------------

by fabpot at 2011/11/07 08:50:59 -0800

@stealth35: The PO is what people will use for their translations. They will then dump it to MO. So, we need both PO and MO loaders and dumpers.

---------------------------------------------------------------------------

by stealth35 at 2011/11/08 01:25:39 -0800

@fabpot, I'm ready for both dumpers, you can merge this, and I'll open a PR for the dumpers

---------------------------------------------------------------------------

by fabpot at 2011/11/08 22:37:47 -0800

I've just had a look at this PR code again and I see that the unit tests are pretty slim. Is it possible to add some tests for the mo loader?

---------------------------------------------------------------------------

by stealth35 at 2011/11/09 01:15:25 -0800

@fabpot test send to @stof ✌️

---------------------------------------------------------------------------

by stof at 2011/11/09 02:22:55 -0800

and merged in this branch

---------------------------------------------------------------------------

by fabpot at 2011/11/09 02:39:09 -0800

The tests do not pass for me:

    There was 1 error:

    1) Symfony\Tests\Component\Translation\Loader\MoFileLoaderTest::testLoadDoesNothingIfEmpty
    InvalidArgumentException: MO stream content has an invalid format.

    /Users/fabien/work/symfony/git/symfony/src/Symfony/Component/Translation/Loader/MoFileLoader.php:79
    /Users/fabien/work/symfony/git/symfony/src/Symfony/Component/Translation/Loader/MoFileLoader.php:46
    /Users/fabien/work/symfony/git/symfony/tests/Symfony/Tests/Component/Translation/Loader/MoFileLoaderTest.php:34

    --

    There was 1 failure:

    1) Symfony\Tests\Component\Translation\Loader\PoFileLoaderTest::testLoad
    Failed asserting that two arrays are equal.
    --- Expected
    +++ Actual
    @@ @@
     Array (
    -    'foo' => 'bar'
     )

    /Users/fabien/work/symfony/git/symfony/tests/Symfony/Tests/Component/Translation/Loader/PoFileLoaderTest.php:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.