[IDP-1183, partial] Add cron-based sync of groups_external data from Google Sheet
#360
Conversation
|
| for ($i = 1; $i <= self::MAX_SYNC_SETS; $i++) { | ||
| $appPrefixKey = sprintf('set%uAppPrefix', $i); | ||
| $googleSheetIdKey = sprintf('set%uGoogleSheetId', $i); | ||
|
|
||
| if (! array_key_exists($appPrefixKey, $syncSetsParams)) { | ||
| Yii::warning(sprintf( | ||
| 'Finished syncing external groups after %s sync set(s).', | ||
| ($i - 1) | ||
| )); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This would be my preference, just because it's more consistent with typical for loops. Leave the 1-based numbering for user-focused text. (Note: there's one more $i further down.)
| for ($i = 1; $i <= self::MAX_SYNC_SETS; $i++) { | |
| $appPrefixKey = sprintf('set%uAppPrefix', $i); | |
| $googleSheetIdKey = sprintf('set%uGoogleSheetId', $i); | |
| if (! array_key_exists($appPrefixKey, $syncSetsParams)) { | |
| Yii::warning(sprintf( | |
| 'Finished syncing external groups after %s sync set(s).', | |
| ($i - 1) | |
| )); | |
| break; | |
| } | |
| for ($i = 0; $i < self::MAX_SYNC_SETS; $i++) { | |
| $appPrefixKey = sprintf('set%uAppPrefix', $i+1); | |
| $googleSheetIdKey = sprintf('set%uGoogleSheetId', $i+1); | |
| if (! array_key_exists($appPrefixKey, $syncSetsParams)) { | |
| Yii::warning(sprintf( | |
| 'Finished syncing external groups after %s sync set(s).', | |
| $i | |
| )); | |
| break; | |
| } |
There was a problem hiding this comment.
That makes sense, but since this is also used for the names of the environment variables, starting with 1 seems more intuitive than starting with 0 in this case. I agree that it's more intuitive/expected that code-only for-loops start with 0, though.
| Yii::error(sprintf( | ||
| 'Errors that occurred while syncing %s external groups: %s', | ||
| $appPrefix, | ||
| join(" / ", $errors) |
There was a problem hiding this comment.
Since it's theoretically possible to accumulate thousands of errors, would it be good to limit the length of this log message?
There was a problem hiding this comment.
Hmm, I hadn't thought of that. Yes, I can limit that error message length.
There was a problem hiding this comment.
I'll do that in a follow-up PR.
IDP-1183 Sync prefixed-groups from Google Sheet to ID Broker's
groups_externalfieldAdded
groups_externaldata from Google SheetFixed
$user->updateExternalGroups()Feature PR Checklist
make composershowmake psr2