The trigger_error php function should not be used, instead use the logger from the framework. I'm providing a MR shortly.

CommentFileSizeAuthor
#4 3218693-4.patch3.31 KBbbrala
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

jurgenhaas created an issue. See original summary.

jurgenhaas’s picture

Status: Active » Needs review
bbrala’s picture

StatusFileSize
new3.31 KB

The line is pretty much a copy of the code in core. I've also checked the code of those 2 methods and i actually think the methods ConfigurableResourceTypeRepository::getRelatableResourceTypesFromFieldDefinition and ConfigurableResourceTypeRepository::lookupResourceType might actually be redundant since #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given has been merged. The code that was placed in extra's has been migrated to core.

I've attached a patch to test that assumption.

bbrala’s picture

Ok i just realized, the patch was not merged into 8.9.x, unfortunate.

Status: Needs review » Needs work

The last submitted patch, 4: 3218693-4.patch, failed testing. View results

bbrala’s picture

Thanks for your merge request. :)

I'm not too sure i want to diverge from the core solution too much. In core it's also a trigger error. When i remove this code when Drupal 8 support is dropped this change to use logger will result in changes in behaviour. I'd prefer we don't do that.

So unless you have compelling reasons to apply this anyways i'd rather not.

jurgenhaas’s picture

@bbrala thanks for your reply. The use case is pretty generic: we have monitoring in place which listens to all the standard logging channels that Drupal uses. That's the logger service almost everywhere and logger optionally stores the log messages in either dblog or syslog. We also have analysis tools that are attached to the logger service and they qualify the log message by certain rules and trigger an alert in a devop platform, where the IT team gets notified and can react as required.

However, you're right that the two functions could be removed completely, then this is not an issue of jsonapi_extras anymore.

bbrala’s picture

Title: Use logger instead of trigger_error » Remove 3 ConfigurableResourceTypeRepository methods when Drupal 8 support is dropped
Status: Needs work » Postponed

Fair enough, i understand your need. But Drupal 8 EOL is pretty soon, so around that time (or a little earlier) a new major release will be published. This will remove these lines from the code :) Until then i won't be changing the code compared to core.

You are ofcourse free to use the patch you supplied in your projects ;)

bradjones1 made their first commit to this issue’s fork.

bradjones1’s picture

Status: Postponed » Needs review

Rebased. This is no longer postponed as D8 is bye-bye.

jurgenhaas’s picture

Status: Needs review » Reviewed & tested by the community

Looks like this has made it to the latest release? I just figured that the patch doesn't apply anymore and the 2 functions have disappeared from the codebase. Maybe just the reference to this issue was missing in the commit message, never mind.

bbrala’s picture

Status: Reviewed & tested by the community » Fixed

Ahh, didn't find this issue, I removed old code paths when preparing the release. These were indeed removed.

Fixing and giving credit, even if it wasnt committed through this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.