Problem/Motivation

> throw new \InvalidArgumentException('Minimum requirement: driver://host/database');

This is unclear.

Would be better as something like:

> throw new \InvalidArgumentException("The given database connection URL '$url' is badly formed. The minimum requirement is 'driver://host/database'.");

Steps to reproduce

Proposed resolution

Change the message to "The given database connection URL '$url' is invalid. The minimum requirement is: 'driver://host/database'"

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3457009

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:

About issue forks

Comments

joachim created an issue. See original summary.

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

ankitv18’s picture

ankitv18 commented

@joachim target branch should be 11.x or 11.0.x?

ankitv18’s picture

ankitv18 commented
Assigned: Unassigned » ankitv18
joachim’s picture

joachim commented

11.x, according to https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-...

ankitv18 opened merge request !8538

ankitv18 changed the visibility of the branch 3457009-exception-message-thrown to hidden.

ankitv18 opened merge request !8539

ankitv18’s picture

ankitv18 commented
Version: 11.0.x-dev » 11.x-dev
Assigned: ankitv18 » Unassigned

MR!8539 is ready for a review.

ankitv18’s picture

ankitv18 commented
Status: Active » Needs review
vinmayiswamy’s picture

vinmayiswamy commented
Status: Needs review » Reviewed & tested by the community

Hi, I've verified MR 8539 in Drupal 11.x.
The change aligns with the requirement outlined in the issue and works as expected.
Since the code quality hasn't changed, +1 to RTBC.
Thanks!

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Thanks for working to improve the exception message!

Issues for Drupal should always have the 'proposed resolution' section complete and up to date as this is important information for reviewers and committers. I had updated that section.

The proposed new text uses the phrase 'badly formed', instead I think it would be simpler and clearer to say that it is 'invalid'. Plus, there are no instances of 'badly formed' in the code.

I have left a comment on the MR.

ankitv18’s picture

ankitv18 commented
Status: Needs work » Needs review

Thanks @quietone for reviewing the MR, I've updated as per suggestion.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed

Will note that the issue was tagged novice for newer users. And the issue was created just few weeks ago.

@ankitv18 based on your post history would definitely say probably have the skill set for non novice issues.

Now there is no official time frame around the tag but I say if it’s sat there for a month or two with no new user picking up its fair game.

Thanks

quietone’s picture

quietone commented

Thanks this look much better now.

Leaving at RTBC

quietone’s picture

quietone commented
Title: exception message thrown in createConnectionOptionsFromUrl() is unclear » Add input string to exception message thrown in createConnectionOptionsFromUrl()

  • catch committed b8c27098 on 10.3.x 
    Issue #3457009 by ankitv18, quietone, joachim: Add input string to...

  • catch committed 7e5fff8c on 10.4.x 
    Issue #3457009 by ankitv18, quietone, joachim: Add input string to...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

  • catch committed dfba5cb9 on 11.0.x 
    Issue #3457009 by ankitv18, quietone, joachim: Add input string to...

  • catch committed 059fd87e on 11.x 
    Issue #3457009 by ankitv18, quietone, joachim: Add input string to...

catch closed merge request !8539

Status: Fixed » Closed (fixed)

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