Account created on 17 November 2008, almost 16 years ago
#

Recent comments

🇬🇧United Kingdom rivimey

Have you updated the vendor/google files recently?

My own calendar is still working fine, so it seems this is not general (unless there is some sort of roll-out going on)?

🇬🇧United Kingdom rivimey

As far as I know there is no specific google calendar flag indicating 'all day'.

I'm not sure what you are asking for, though?

🇬🇧United Kingdom rivimey

I've had a look through the changes as a whole, and they seem ok except for two thoughts:

  • The Core Version Requirement line could be optimised a bit using > and < version operators.
  • I usually try to avoid calling things "New-X" because inevitably their newness degrades over time... would it be possible to rename the new class by drawing from the new differences?

I would really like to see this committed ASAP because it's part of a complex version dependency ring on my current site.

🇬🇧United Kingdom rivimey

This should be a backport of the equivalent changes on the 3.0.x branch, found in commit 40331ac3cba9929864f4a1c0c49f636a3af23811

I have committed this to both 2.x branche but have removed the 'if no display name use email' as this could be considered a bug by some users. I suggest such changes are added in a hook, if required.

[The 3.x branch already included this.]

🇬🇧United Kingdom rivimey

Hi, thanks for your willingness to contribute. It has always been a stretch goal for the module to include this functionality but my current use-case doesn't require it, and it is a significant change. I would suggest that it may be best implemented as a submodule or some similar structure. It would almost certainly be appropriate to do the work on a 4.x branch.

I hope you have noted the (older) issues where this topic is discussed in more detail, which will help you with an outline of what needs to be done, most importantly #3082332: Moving the admin settings to an entity (user, group, or sitewide) so that I can connect multiple google accounts , though #3110585: Improve cleanup logic may also help.

I can give you some assistance but I am doing very little Drupal work at present so that assistance will be mostly responding to questions. If I don't respond here please feel free to contact me by email via the account Contact page.

🇬🇧United Kingdom rivimey

The module works fine using google_api_client - has been running in production for months.

I have done what I intend to do... any further work should be submitted as patches.

🇬🇧United Kingdom rivimey

The accessCheck tests are included in the 3.0.x branch, which is the future of the module.

The patch here uses accessCheck(False) but in many/most cases should be using accessCheck(True).

Please backport the 3.0.x changes if you still need the 2.x branch of the module.

🇬🇧United Kingdom rivimey

The future of the module is using the google_client_api module, and this doc is specific to the old google_secrets module. The documentation needs update to the 3.0.x branch.

🇬🇧United Kingdom rivimey

I (as maintainer) have no intention of supporting D11 with this module, as noted on the project page.

🇬🇧United Kingdom rivimey

In #3 you mention the stretch goal -- I would suggest that this and a number of other policies (that make sense) are implemented to ensure that the API is in fact sufficient to implement them, even if those additional policies are later left as guide examples or even abandoned. With this sort of API it is really hard to ensure you have all the right interfaces and the right data available up front...

🇬🇧United Kingdom rivimey

Re #333 I agree. It sounds like a good plan, especially as those who care can reinstate the old behaviour in a forward-compatible way.

🇬🇧United Kingdom rivimey

@shubham_jain the -18 patch looks reasonable, thanks.

@MegaChriz would you be able to test it at all?

🇬🇧United Kingdom rivimey

@shubham_jain the next thing to do is:

Marking Needs Work because of the core_version_requirement change needed (remove it in favour of #3364838: Need to be D10 compatible)

If you could post another version of the change that excludes changes to the .info.yml file, then I can apply it.

🇬🇧United Kingdom rivimey

@shubham_jain we all make mistakes, so no problem. I explained at length to enable you to understand, that is all.

I would encourage you to fill out your d.o profile a little more, but of course that is your choice.

🇬🇧United Kingdom rivimey

The patch looks sane, but @shubham_jain please Do NOT post a single patch to multiple issues - it just wastes everyone's time thinking there are several different issues, and should an error or improvement be found in one issue's patch the other issues probably won't be kept in sync.

The correct action is to Close(Duplicate) all but one of the issues, with a comment indicating the remaining issue link and in that remaining issue add your patch. Anybody can close (or open) an issue - it doesn't require the maintainer to do so.

Also, I note that you have included in this patch the change to the core version requirement. It is generally a good idea to stick to fixing one issue in each patch, but moreover, the change just adds D10, while I would expect that the current module is no longer compatible with D8 (especially because of the need for 'once'. When modifying shared codebases it is important to do the whole job, not just the part that lets you move on.

Marking Needs Work because of the core_version_requirement change needed.

🇬🇧United Kingdom rivimey

Just a note: I think it unlikely that the module is compatible with both D8 and D10, and quite possibly not D9.0 and D10 either. I have been marking D10 modules with "D9.5 || D10" lately unless I know better.

🇬🇧United Kingdom rivimey

I apologise for not getting the translation right already and thank you for your work on this.

However, the patch here has a lot of changes and while I can see some are related to translation, some are definitely not. In addition, the replacement for the config/install views file seems to be quite different from the original, not merely an update.

Could you clean up the patch somewhat? strip out anything not necessary for the translation to work, and rework the config/install file to be a minimal update also? It would be easier for me to check if there were several patches that grouped the different sorts of change, even if they all need applying at once.

My last concern is that I see the base table name is changed. I presume this is a necessary thing, although I have no idea why it would matter. This change would seem to at least minimally require an update hook to adjust existing installations. There are perhaps other changes that require an update hook's intervention also?

Finally, note that the 2.x branch is now on security-fixes only - it exists purely for those not willing to change from google_secrets.

🇬🇧United Kingdom rivimey

I'm not sure of the benefit of removing the link from the import, and I don't know conclusively if a private link is indeed useless, or just requires the right conditions. Consequently I am not minded to apply the patch. Can you explain the reasoning further?

🇬🇧United Kingdom rivimey

@mortona2k, I am no sort of expert on views data arrays but if this patch works, it would make sense to me to extend the scope a bit to include other fields, such as end date, and also to include time.

🇬🇧United Kingdom rivimey

Thanks for identifying the fix.

@edmargomes I don't really understand comment 7, can you elaborate?

🇬🇧United Kingdom rivimey

In the deprecation notice for core/popperjs at [#3307518] reference is made to "Floating UI". Can this be dropped into barrio instead?

🇬🇧United Kingdom rivimey

Sounds like two actions are needed:
1. add (a version of) the code in #44 as a test case, coupled with other rational variations if any.

2. update the patch to permit (1) to be run, i.e. creating display programmatically.

For (2) I suggest replacing the new 'throw' with setting a flag on the EntityDIsplayBase object signalling that dependencies are out of date, and then on save() if the flag is set then update the dependencies and _at that time_ throw the exception, as I would expect by then everything to exist.

If this doesn't work I'm somewhat stumped - the exception is thrown when the entity display for this display mode can't be loaded, so in what circumstances is it ok to have a display config referencing an unloadable mode config?

🇬🇧United Kingdom rivimey

@portulaca It sounds like you need a 'use' statement for LanguageInterface in the file that you have added the code from #10 to. I can't recall the exact place but something of the form:

use \Drupal\Core\...\...\LanguageInterface;

Search existing code for examples.

🇬🇧United Kingdom rivimey

An initial patch to start with

🇬🇧United Kingdom rivimey

7 years, 2 major Drupal versions.... sigh.

Having looked at the last review which set needs work and the latest patch, which addresses the only critical change mentioned, I feel this patch is overdue for Needs Review. Given various people have been rerolling it it seems the patch(es) are being used, and so quite possibly this should be RTBC. One step at a time though!

Production build 0.71.5 2024