🇬🇧United Kingdom @oily

@berdir I am not sure why line 212 of Renderer.php has /* array */ instead of typehinting the $elements parameter 'array'?

public function render(/* array */&$elements, $is_root_call = FALSE) {

#29 +1. I realised I had come across this before with Symfony textarea field. I wanted to allow the user to enter a list, actually Black Cab runs which students of the Knowledge study and memorise, for example,

Leave on Left Green Lanes
Right Highbury New Park
etc
etc
etc
Forward into Gibson Square

All the runs follow a similar pattern, but they vary a lot in length (how many lines in total).

You have a choice to either present one textbox per line or a textarea for all the lines. Thinking it through I realised that the newlines are easily handled by explode() on the TEXT (string) value. Then custom validation can be applied to the array. The flexibility of the textarea field type, including the length of the field value, is what seems to make it so useful.

I think there might be edge cases where a maxlength value is desirable. But IMO it is best to leave the core textarea field as it is. Developers can create a custom field or use custom validation. But such cases will be relatively infrequent.

@godotislate I have looked at your new kernel test coverage. Can you please advise me if you think I should refactor my test coverage 'borrowing' from your code to take account of new lines? I am not sure my test coverage is currently fit for purpose?

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

#39 +1. Are we ready for RTBTC? We have test coverage in place, CR seems done.. Pipeline green?

I have had a look at the CR and the MR and recent comments. I note there are 2x todos, 1 in User.php and 1 in Workspace.php. It would seem a follow-up issue could deal with those? Since the 2x timestamps involved in the todos are unusual and need to be handled differently since we are allowing safe handling of NULL values for timestamp in this issue but these 2x todos are when NULL is not currently supposed to be allowed.

Worth taking a look at the phpdocs for the 2x timestamps to see why they should not return NULL and if should throw an exception or how null values are to be dealt with.

Some work needed on the test case. Add newlines to the random text and test how that affects the length?

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

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

Fix merge conflict.

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

Test-only test now fails as it should. Other tests in the pipeline are breaking. Not sure if related.

Fixed PHPCS. Rebased. Now several tests seem broken in pipeline.

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

Applied several of @longwave's suggestions, PHPSTAN/CS

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

@smustgrave. I agree. I created a code review as others here did. My review was to change 'string' to 'callable'. I believe my review was ignored while everyone else's was resolved. No idea why.

#49 @smustgrave Because the typehint should not be 'string' it should be 'callable'. If you disagree, please explain why.

Self-RTBTC

Pipleline green.

Change to Needs review.

@acbramley I used the online single file editor. Weird.

Not sure why pipeline failed. Failures dont seem related. It has failed on a one word commit.

Re: #64 Can you please explain why. Please reference the comments I have made. A reasoned response is much appreciated.

@xjm Thank you for #42 but I respectfully disagree: I have been constantly changing my 'point'. It has evolved.

The status is 'Needs review'. Distilled from the above is this:

At line 493:

string|NULL should be callable|NULL

If others disagree then I am keen to hear their viewpoint.

If the callback at line 493 is 'invoked' then it should be typehinted as \Closure, not string|NULL.

Thank you for #36 @xjm. Please refer to #30:

A PHP callable is a PHP variable that can be used by the call_user_func() function and returns true when passed to the is_callable() function. It can be a \Closure instance, an object implementing an __invoke() method (which is what closures are in fact), a string representing a function or an array representing an object method or a class method.,

That seems to be a clear description of PHP callables. It explains the variance in Drupal docs you have highlighted in #36.

The action to take when documenting a callable is straightforward:

1. Is it a \Closure? If so it must be an object implementing an __invoke() method. So it is 'invoked'..
2. If not a \Closure and does not invoke an __invoke() method, is may be a string representing a function?
3. If neither of the above, is must be an array?

Line 508
throw new \InvalidArgumentException('Default value callback must be a <strong>string</strong>,...

makes it clear that the $callback parameter of BaseFieldDefinition::setDefaultValueCallback() is a string callback (see item 2).

Therefore, it is 'called' not 'invoked'. Callables elsewhere e.g. in Drupal API docs (see item 1) are correctly described as 'invoked'.

Re #32 "Plus addressing @joachim's feedback". So why not addressing my 'feedback', too?

#31 @xjm

@oily, Thanks for your interest in this topic.

Everyone else has 'contributed' to this discussion.

... the discussion is out of scope for this issue

How is it 'out of scope'?!

@xjm You are correct that in Drupal API docs eg:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

there are:

callable $callback: A callable that invokes a hook implementation.

But is that the purpose of the callback in this issue?

If not then best to contradistinguish the chalk from the cheese?

@xjm If you are unconvinced by my arguments (code review), do you want to do a follow-up to change the few instances of 'calling' callables' in the existing Drupal codebase to 'invoking'?

Added code review: callables: called or invoked.

Chaging to needs work after creating a review of the code. A couple of perhaps nitty suggestions on code comments.

Resolved merge conflict. Rebased.

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

Added test coverage. Rebased. Not sure why tests failed.

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

#59 "..To ensure that all participants are heard and respected, we encourage a brief pause from the conversation to gain perspective.."

So why #60? I am following #59. Suggest the others involved do the same.

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

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

RE: #54 'This one did have a conflict in OpenTelemetryNodePagePerformanceTest, which it seems like you fixed, but then the fix failed tests.'

I do not know how to fix the tests. Anything wrong with that?

@tim.plunkett Not interested in your opinion.

@quietone So revert the commit.

You were dressing up with a thin veneer of politeness impudent and unwelcome advice. Now that the veneer is removed. And you try to discredit my contributions and my professional image in front of a potentially wide audience. I have no problem with the rules.

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

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

@smustgrave Thanks for reverting I agree with it (in hindsight)- seemed a good idea at the time : ).

@acbramley I am sure it would.

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

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

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

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

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

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

oily → changed the visibility of the branch 3453331-recipeconfiginstaller-should-process to hidden.

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

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

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

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

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

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

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

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

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

@aaronbauman you also probably need to create a new 11.x branch and merge into that. Core maintainers then decide which branches to port to.

There is a gitlab template that can be used in projects. Can use it at default settings or vary them. I think most projects will have test-only test in pipeline unless the maintainer had a good reason to exclude it.

A separate 'test-only' branch is not necessary. There is a 'test-only' manually activated test in the pipeline.

oily → changed the visibility of the branch 3532360-TEST-ONLY to hidden.

oily → changed the visibility of the branch 3532360-TEST-ONLY to hidden.

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

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

Reduced PHPSTAN errors.

oily → changed the visibility of the branch 3496369-preload-without-cache to hidden.

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

oily → changed the visibility of the branch 2339235-remove-taxonomy-hard to hidden.

Solved merge conflict. Synced branch with main dev branch.

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

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

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

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

@julio_retkwa If you want to have another go at the test coverage, good if you can start by merging your test into the active branch. Can then run manually the 'test-only' test which will fail if test coverage is working well.

RE: #14 Trying a merge request to see if the active fork/ MR is syncing with the 11.x dev branch. It is currently about 90 commits behind it.

@julioretkwa I am trying to get the pipeline to show green with a minor PHPSTAN fix. Your test coverage in your new branch can be ported over to the active branch, then. Then can make sure the test works correctly in the pipeline, too.

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

@alexpott Thanks for considering my suggestion. The CR looks good!

Reviewed CR. 'materialized' views is interesting. Just looked it up. Need to research further. Asking myself 'how to create material view as opposed to a normal view..' Could CR benefit from example Mysql query to create a material view? The example is the IS is: "CREATE VIEW test AS SELECT * FROM users;" That does create a 'material' view?

@acbramley Okay 'opening separate browser tabs' might be I was thinking of a nightwatch test. I'll have another review of the latest. My suggestions were nitty.

@acbramley Okay with the test. Perhaps pedantically I thought implementing a new interface would mean a unit test. I support a kernel test if ticks the box.

Test coverage in place. Test-only test fails as it should.

Can move to RTBTC?

@alexpott The commit I queried.. I understand the purpose now. Line 688 reveals the relevance to this issue. If i had scrolled up a bit I would have seen it.

+1 #27

@alexpott

I added code comment to: https://git.drupalcode.org/project/drupal/-/merge_requests/12442/diffs?c...

Is that commit necessary for filtering views amongst tables?

If we could add minimal support for DB views into core say by adding createView() method, possibly related CRUD methods too. Then do minor refactor of existing test functions eg the one that tests tables all have primary keys. So the test function would do a 'it has not got a primary key. Is it a view? Yes? In which case, no problem.'

The core Schema.php has a method createTable() but has no equivalent method to create views. The tests rely on createTable(). Not sure it is worthwhile or feasible to support views in the piecemeal fashion of this issue.

If we are going to take cognisance of views should be not build it into the database api including the core Schema.php. As it stands we will need, i think, to use raw sql queries in the tests ie CREATE VIEW statements as mentioned in the steps to reproduce.

Okay, I see that the errors in the IS are triggered because views do not need to contain a primary key. There is existing code in other test functions involving the presence/ non-presence of primary keys. Possibly test coverage should be by extending such functions. Or perhaps can incorporate the primary key test code into the new test function..

If test table (view) contains no primary key, check if findTables() 2nd parameter is set to TRUE. If it is, check if the table is a view. Assertion passes if it is a view with no primary key..