Skip to content

Conversation

@Thomasoj
Copy link

Hi,

some SMS gateways require text of the message to be divided into individual parts. I think it was nice to have a divide text within this library, cause some parts of code are same or similar.
I saw PR #9, but I think some parts of code, like counting size of one part are unnecessary. PR also contains a bug that may return a different encoding for each part of the message.

I have prepared my own proposal, which only divide the message into an associative array.
Dividing of message to parts is lazy, so parts are compiled only if you need them in your project.

Tomas

@Thomasoj Thomasoj changed the title Feature: Divide of message to parts Feature: Divide message to parts Dec 30, 2021
@atymic
Copy link

atymic commented Jan 31, 2022

I think both functions are helpful, but we should try to condense the code into a shared function if possible. Maybe worth adding tests to cover the code if @chrisminett is happy with the concept?

Couple of Qs:

  • How does the part splitting handle multi byte chars that traverse messages?
  • With the bug, can you please comment on my PR where it is? We use this code in prod so bugs are bad :p

Thanks

@chrisminett
Copy link
Member

@Thomasoj thanks for the suggestion. I hadn't previously seen any need for separating the message parts, but can see that it might be necessary if using SMPP or similar, and it does make sense to have the logic here where the code already understands the part splits.

I'm not sure about iterating it again to split, it feels like a lot of repetition, and I think there will be the issue that @atymic mentioned that it misses the issue in #8 .

How about something like this:

  • Change initial inspect() that is finding the size, to build an array of message parts while it's iterating the characters.
  • Parts would then be immediately available to new getMessageParts() method.
  • Proposal for truncate() in feat: truncate to length + tests #9 could simply return a portion of the array.

If adding in this extra behaviour for building an array of message parts, I'd like to see the two iterations currently used in inspect() changed to just one. Should be easy to know which encoding is needed up front by checking for the presence of non 7-bit chars in the original message.

@Thomasoj @atymic what do you think?

Lastly, I'm not a fan of the 1-indexed array. Numerically-keyed associative arrays will be treated by PHP as a regular indexed array, so (m)any array manipulation functions in PHP will re-index the keys. I think it's cleaner to simply use $messageParts = [] and $messageParts[] = ....

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