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

Prevent infinite loop and use correct subscribers method #6

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

ash-jc-allen
Copy link
Contributor

Hey!

Sorry for bombarding you with PRs across the different repos all at once haha!

This PR fixes a possible infinite loop that I've come across while working with the Laravel SDK.

First off, apologies if I've done any of this wrong. If it's something you might consider merging, please give me a shout if there's anything you might need changing 🙂

Context

I have a Laravel artisan command that I run every day on my site that grabs the total amount of subscribers for my newsletter and then stores it in the database. I'm currently using Revue but I'm migrating over to using Mailcoach.

At first, I tried using the following code, but I kept getting 0 as the count:

$subscriberCount = Mailcoach::emailList(config('services.mailcoach.newsletter_id'))
    ->activeSubscribersCount;

I'm not quite sure why I was getting 0, so I moved on to try a different method instead:

$subscriberCount = Mailcoach::emailList(config('services.mailcoach.newsletter_id'))
    ->subscribers()
    ->total();

The call to subscribers is where the issue occurs.

The Issue

It seems that the subscribers method calls itself and ends up in an infinite loop which eventually breaks.

I copied the approach used in the subscriber method in the EmailList and passed the subscribers call through to the $this->mailcoach object instead. This appears to then fetch the correct data as intended.

I also updated the method's return type so that it would return a PaginatedResults object instead. I don't know if this is okay to do, or whether it would have been better to return an array instead to prevent the breaking change.

Hopefully, the changes I've done are all okay. But, as I say, please give me a shout if there's anything you might want me to change if you think it's worth merging 😄

@freekmurze freekmurze merged commit 87c5f15 into spatie:main Jan 4, 2023
@freekmurze
Copy link
Member

Nice one! Thanks!

@ash-jc-allen ash-jc-allen deleted the fix/infinite-loop branch January 4, 2023 10:45
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.

2 participants