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

Ability to output in different formats #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

5zymon
Copy link

@5zymon 5zymon commented Mar 15, 2019

This update is bigger in compare to the previous one, so you may want to take a longer look before merging :-).

I needed output to CSV file so I modified google shopping container - which I think was prepared for functionality like this in future. I've separated feed class into formats classes one for XML and one for CSV. Core functionality stayed the same, I am using this library in other module and without making any changes in the module, it generates same XML output as before, so I assume all is good here I didn't break anything. But now when you enter 'csv' in the container, instead of null, you'll get CSV as an output.

It also requires new lib league/csv, which requires php 7 so I had to move the require in composer.json up from php 5.3.0 (which isn't that bad anyway I think ;-)).

@lukesnowden
Copy link
Owner

You’ll have to leave me with this one, big commit. When I get a free hour I’ll take a look over it

@5zymon
Copy link
Author

5zymon commented Mar 15, 2019

No rush and no worries! 90% of this is your code moved around so it can be shared for different formats :-).

@lukesnowden
Copy link
Owner

Sat in pub having a look over it looks good but I’ll wait until I’m back at my Mac to test t👍

5zymon added 2 commits May 24, 2019 06:40
Updated package name
updated composer.json
@Fredyy90
Copy link
Contributor

any updates on this old pull request? i would really like to see an output option to get facebook compatible csv

@lukesnowden
Copy link
Owner

Have you pulled this PR down and tested it works?

@Fredyy90
Copy link
Contributor

no haven't checked it, I think it might need fundamental rebasing, as its over 3 years behind the current versions.
i might take a look at it later, we currently run in the issue, that our feed is multiple gigabytes big and the xml overhead is an issue we would like to avoid. so an output option to get facebook csv would be exactly what we need.

@lukesnowden
Copy link
Owner

If you pull it down and it all works as expected let me know and I'll merge it in as a new major version release

@Fredyy90
Copy link
Contributor

I forked @5zymon repo and tested it with my current setup.

I didn't rebase it yet to also get all last 3 years changes, since I don't know how to "claim" this pr or how to fork a repo and rebase it on another repo.

But @5zymon PR works as expected, you need to set the format you want first, when configuring your feed. If you don't want to use the default format (XML).
GoogleShopping::$container = \LukeSnowden\GoogleShoppingFeed\Formats::CSV;

After setting the format, everything else works as before, you also have to run GoogleShopping::asRss(false); to get the output. This is maybe the only "unlogical" thing in the PR as the function will no longer output RSS when changing the format before.

But changing the name of the function will also result in a breaking change. Potentially, we can rename the output function to export() or output() and keep a second function called asRSS() for backward compatibility, that just calls the new renamed version.

    public function asRss($output = false)
    {
        return $this->output($output);
    }

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.

3 participants