🇦🇺Australia @dpi

Looks like interface is required now.

https://www.drupal.org/node/3417034 →

Thanks, this should be pretty straightforward.

Mostly copy/pasting and mirroring the existing hook pieces but for alter.

MR welcome. Thanks for the doc, I think thats critical to this issue.

Tests not required for this change.

These are of course implementation details, and perhaps also class misuse of terminology in Drupal-land.

But an alter isn't a hook. Alters are called differently to hooks.

I have also seen in rare cases where some hook was named with a _alter suffix, but called using the hook/invoke code path. Which would in Hux' case require you to use #[Hook] attribute. Leaving to even more #dxwtf.

Re,

[Hook] does not work as expected [Alter]

I think thats just something you get used to during learning about Hux. There is a clear delineation between hook and alter. Otherwise the design of Hux would just have one universal/multi-use Hook attribute.

I think at best for this issue, I'd consider a trigger_error or something when __constructing a #[Hook] instance, looking for str_ends_with(_alter), so at least you'd be able to patch things and deal with modules which are misbehaving w MH->invoke/alter.

Let me know your thoughts, but for now my thinking is wontfix.

As found in 1.4.0

Thanks, wholesale replacement also seems viable and sensible 👍

Yep, confirmed.

I think we should coordinate a queue or message → based solution, with a shared solution for ✨ Nominate a different user than uid1 Active

dpi → made their first commit to this issue’s fork.

Looks like resolved elsewhere...

Thought I'd mention, with this change: it seems since the core stuff comes back, modules like Simple Recaptcha → inject themselves once again. So on CI/site test runs, people may see failures related to CAPTCHA. In my case, I've opted to disable CAPTCHA on testing environments.

Could we get this guy in a tagged release? :)

dpi → created an issue.

_serviceId was deprecated in #2531564: Fix leaky and brittle container serialization solution → in 9.5 and marked for removal in core 11. The property was was an internal only thing anyway.

Looks like your third party code was relying on this internal property, or you have an outdated patch to core which is using it.

Take some time to look through all your patches and contrib and adapt it to work without this property, and make you're not making use of the container serialization features in #2531564: Fix leaky and brittle container serialization solution → .

dpi → created an issue.

Overall proposal looks fine, however I've modified the MR to be a little more correct.

dpi → made their first commit to this issue’s fork.

dpi → created an issue.

Just the unrelated Nightwatch failure now.

I’m pretty sure quotes are allowed here. It’s a YAML string.

And to be clear, this isn’t my code: https://git.drupalcode.org/project/key/-/blob/8.x-1.x/key.info.yml?ref_t...

It’s a perfectly valid constraint, which gets clobbered by naive code.

Noticed it in this pipeline for an MR of Key

https://git.drupalcode.org/issue/key-3470903/-/jobs/2586157

It’s my MR, but not my project.

Test fail seems unrelated...

The "phpunit (max PHP version) " job for this is not passing.

The latest "phpunit (max PHP version) " job on HEAD was passing, last run a month ago.

Notably, the last run was on 10.3, this new run is now 11.

Seems like my changes are unrelated.

I've seen the printer class error on other projects.

A variety of constructors + plugin factories made final.

Anyone extending these services should use service decorators combined with setter injection.

This allows Key to be more maintainable.

dpi → created an issue.

dpi → created an issue.

dpi → created an issue.

If the view tab is present, and its redirecting you to not found, I'm inclined to believe its some kind of issue with custom code or contrib.

In core, if you dont have access to a view page. The "View" tab won't show, because access control ensures it. The same applies if views are controlling access to the View page via entity_access.

Re #12, {taxonomy_vocabulary} table is a D7 concept, the article incorrectly shows it as a "Drupal 8,9 and 10" feature. Vocabularies in D8 and later are config, which don't have or need a dedicated table.

Reading through this issue, it doesnt make sense to me as a core bug. So will need some investigation on your own. Closing based on this, and this issue is quite stale with no followers.

I’ve pretty much adopted this as a pattern for the last year or so, it’s great

In case it’s not obvious, you can also access services by class name, since the proposed convention is simply making the service id the same as the backing class/interface.

\Drupal::service(Drupal\Core\Entity\EntityTypeManagerInterface::class)

Even D11's views-mini-pager.html.twig template doesn't have this.

Going to close this but feel free to reopen with more detail.

Link in IS

One noticable thing that can clash with common frontend conventions is its bringing in Claro's css/components/button.css via claro/global-styling. This CSS file has a `.button` class, with quite a lot of properties.

When in a Gin Layout Builder edit page, buttons on the page can go out of whack because of this common class name.

Even things like CKEditor5's stylescombo, which is built into core, can bring in bad classes if a `button` class was allowed.

Root composer.json requires drupal/gin_lb 1.0.x-dev@dev

Update your constraints to use non-dev, like ^1@rc

You could composer remove drupal/gin_lb, then composer require drupal/gin_lb to make it simple.

Quick reroll

We might be a bit far down the road on 2.x at this point.

Perhaps major changes should be delayed to a new major.

I'm having the same issues as others have described. This is a major issue that has been open for over a year? I understand and appreciate those that volunteer their time to build and maintain contrib modules, but could you at least put a disclaimer on the main page warning people not to install this anymore? Are there any manual steps that can be taken to cleanup the mess?

This is a collaborative effort. You may choose to participate in contributing code, or you may evaluate the proposed changes at your own risk.

Clearly, there is unaddressed feedback, that is why it remains unmerged.

Issues like these may of course be worked around manually, without code changes. Again, at you own risk.

Otherwise, you can put up with "the mess" that is having one additional unused module installed.

Thanks.

As found in 3.8.0-beta1

dpi → created an issue.

Thanks everyone for continuing to push this along.

Theres some code gripes, for which I'm happy to address myself.

My major concern (cue bikeshedding) is the icon as proposed doesn't really capture the concept of iframe.

It is much closer to something that would represent a popup, since it seems to have the Mac-like three dots a window would have. But as we know, iframes do not have a window-border.

Some thoughts for an iframe: box with address bar or scroll bar. Use some kind of HTML-concept as the box concents, like a _generic_ tag: </>

There's a variety of concepts, alongside the one proposed, found when using search on icon websites/image search.

Lets try to keep rounded-corners/stroke-width similar to that found in the default Drupal/CKE icons. Mono/uncolored is great.

Thanks!

Thanks @marcoscano, sounds reasonable to me.

Feedback and questions for maintainers. Nothing actionable by non-maintainers.

Thanks!

dpi → created an issue.

4.2.x has D11 support.

Found in 4.2.0

As found in 2.0.1.

Time has passed, no need for this any longer.

This does sound like a good idea, however I'm not sure if it makes to bring it into the module.

Its something that would require maintenance, and expire after a few years.

Perhaps a guide or blog post, with code examples as a separate (Github) codeset?

Happy to advertise something on the project page.

I dont think there is anything actionable here on the project side, closing.

Yeah I'm not exactly sure what triggers it, but you can see the `bot` flag in a user extract:

{
  "id": 24510,
  "username": "longwave",
  "name": "Dave Long",
  "state": "active",
  "locked": false,
  "avatar_url": "https://git.drupalcode.org/uploads/-/system/user/avatar/24510/picture-246492-1701276520.jpg",
  "web_url": "https://git.drupalcode.org/longwave",
  "created_at": "2018-12-11T21:35:54.623Z",
  "bio": "",
  "location": "UK",
  "public_email": "",
  "skype": "",
  "linkedin": "",
  "twitter": "",
  "discord": "",
  "website_url": "https://www.drupal.org/u/longwave",
  "organization": "",
  "job_title": "",
  "pronouns": "he/him",
  "bot": false,
  "work_information": null,
  "followers": 0,
  "following": 0,
  "is_followed": false,
  "local_time": null
}

This flag seems to also be tied to the inverse `humans` and `exclude_humans` filter on list endpoints https://docs.gitlab.com/ee/api/users.html#for-non-administrator-users

-

Surely the bot flag is not powered by a username suffix?

See 4

This also moves use of config out of constructor. Its by convention a config isnt stored from the constructor, its accessed from configfactory on demand as its state can change during the lifecycle of a request.

We can also fix up https://drupal.org/node/3395436 for autoconfiguration, and remove use of ContainerInjectionInterface.

dpi → created an issue.

Being about to search the Gitlab API on the `identities` properties could also work.

However, this data is not public data unless you are specifically requesting the current user.

E.g for my user:

identities	
  0	
    provider	"jwt"
    extern_uid	"dpi@81431.no-reply.drupal.org"

But this data is not visible for other users, or is searchable/filterable on the `/users` endpoint as far as I can tell.

All for this, I find it frustrating that the wider PHP community has adopted in Stan / Psalm / others.

Recently the PHPDoc maintainer said something relevant to this topic:

https://phpc.social/@jaapio/112797821284066042

I'm going to try to reboot the working group for @phpfig's psr-5. Goal is to write down the standards we are already using in @phpstan @phpdoc @phpstorm, @psalm and many others.
I'm looking for people that will help me once in a while to check if I do not write down things that will break these amazing tools.

Some projects have been left behind by recent innovation.

We should just allow anything, and let our approved static analyser (PHPStan) validate it. Its not really within the domain of Coding Standards to do validation.

See also, the discussion at #3309010: Support PHPDoc Types in @param @var @return annotations →

Small IS rewrite.

Should already be fine since constraints were using gte. Merged with tighter constraints.

Fixed conflicts for 10.2 and 11.x and created a MR for 11.x

dpi → changed the visibility of the branch 11.x to hidden.

dpi → changed the visibility of the branch 3342398-path-module-calls to hidden.

Same with https://git.drupalcode.org/tugboat

Others known?

dpi → created an issue.

Thanks. Happy with this headline-satisfying change.

The block remains less than ideal, lets tackle other changes in follow ups.

dpi → created an issue.

PHPStan level 5 is required for it to show up.

This is what you should expect to see:

phpstan: Parameter $xyz of method Class::method() expects 'blue'|'green'|'red', 'purple' given.

PHPstan does the checking to see if it is well formed, and validates i/o.

Coding standards applies coding standards. If after a while we find some kind of variants in how this syntax is written, then we go through the process of implementing sniffs.

Approving all data types in coding standard upfront slows down progress.

FWIW, string and pipe is already allowed. I believe from convention our standard doesnt allow spaces-between types. So it is merely the literal-string syntax I understand as the only disagreement.

Why would coding standards need an update?

I don't think we have that anywhere else in core

Psalm / Stan syntax have pretty much been unofficially approved by the PHP community.

Next steps would be to convert to enum and use that in the @param documentation.

This is a quick fix, the enum issue is certainly more involved than this.

I must say I don't particularly like this, the enum would be much nicer.

This doesnt change anything, and I agree the enum is the next step.

Regardless, I think it's possible (even if ill-advised) to alter or extend the available options, so this might be needlessly restrictive.

This is an underscore-function, so its designated internal. It has a list of allowed values, its just not enforced. Adding restrictions would involve actually enforcing these values, which this issue does not propose. The other enum issue however, would actually enforce the values. So perhaps its worth adding your 2c there.

I am concerned on how we’d approach dealing with the things enums do for us. It do you imagine an object class magically gets bound to an enum, without an explicit Definition attr, or something?

We can support this scenario alongside existing.

Pinto/Objects namespace?

Probably configurable alongside the existing namespace container parameter.

Should a new attribute register a class as an object, or pick up the existing attribute?

I’m sure there are internal design issues that need to be resolved in order to couple from enums, while keeping existing functionality.

I wonder if we could add a property attribute e.g #[Theme\Variable] and put that on constructor Params and have the theme hook variables inferred

This sounds like an interesting idea.

New issue, though. This one isn’t quite it.

Trouble is Pinto as it is, is flexible so you can have objects located anywhere. If you look into internals, you’ll find the file name of the class is picked up with reflection. Even outside of regular src/ discovery. It’s not efficient to scan every directory in a project, so this is what we have.

A solution could be to have a rigid, scanned directory/namespace just like Pinto enums do right now.

This is kinda related to https://github.com/dpi/pinto/issues/4, putting all the metadata in the enum. Maybe solves issues with metadata spread all over? So your objects become purely about input/output.

Just hit on this again in another project 🤦

https://git.drupalcode.org/project/role_enum/-/merge_requests/2/pipelines

Lucky I had a bunch of code in .gitlab-ci.yml / .gitlab/custom_symlink_project.php I could copy 😇

dpi → created an issue.

dpi → created an issue.

If changing the min core, may as well be 10.2 (current min w/ sec support)

Thanks!

Thanks!

Thanks! This is what I had in mind.

My preference is MR !106, though I think proper messaging should be added in the other existing catch blocks, and the use of HTML and faking messenger is also gross. Out of scope.

The whole message

> The source path %path is likely a valid path. It is preferred to create URL aliases for existing paths rather than redirects.

May not even be correct as it is not programmatically determine if its a "valid path", using language like "likely".

I'd rather not display the message at all if the user cant access the link.

This can be done with $url->access rather than permission checks, which are not enough when custom access control is added on a route.

dpi → made their first commit to this issue’s fork.

Reformatting, retitling