Skip to content

[4.0] Banners - nullable datetime fields#25360

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
brianteeman:fri1
Jul 17, 2019
Merged

[4.0] Banners - nullable datetime fields#25360
wilsonge merged 2 commits intojoomla:4.0-devfrom
brianteeman:fri1

Conversation

@brianteeman
Copy link
Copy Markdown
Contributor

Pull Request for Issue #16788 ....same as #24675 for contacts - I really just copied the work of others (someone has to)

Summary of Changes

some datetime fields should be nullable as default

  • publish_up
  • publish_down
  • checked_out_time

Testing Instructions

test com_banners

Expected result

banners should works as before

Pull Request for Issue joomla#16788 ....same as joomla#24675 for contacts

Summary of Changes
some datetime fields should be nullable as default

- publish_up
- publish_down
- checked_out_time

### Testing Instructions
test com_banners

### Expected result
banners should works as before
@brianteeman brianteeman changed the title [4.0] Baanners - nullable datetime fields [4.0] Banners - nullable datetime fields Jun 28, 2019
`checked_out_time` datetime,
`publish_up` datetime,
`publish_down` datetime,
`reset` datetime NOT NULL DEFAULT '0000-00-00 00:00:00',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reset can be null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just copied from the example. So I don't know

Copy link
Copy Markdown
Member

@richard67 richard67 Jun 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alikon In your PR #24675 , which is the template for this here, you handled only columns checked_out_time, publish_up and publish_down. What do you think, should Brian continue like that, or should he handle also other columns which should be nullable? From a functional point of view, the reset column here should be nullable, too, for the case if there never has been a reset. On the other hand, if in all other PRs right now only the 3 columns mentioned above are handled, this PR here would be consistent with it, and we would know we have to check for other columns later again for all components. My opinion is that it should be consistent, so this PR is ok for me now. But I'd like to know your opinion on it, too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, different tables have different fields/behaviour, that said, following this comment as only an example #24675 (comment) and without a proper effort to try to move forwrad joomla db schema from mambo times ....... (sorry for the rant)....for me too the reset field should be nullable......but again without a full re-think about db schema ......

p.s
thanks Brian

@ghost
Copy link
Copy Markdown

ghost commented Jun 29, 2019

@brianteeman i guess #16788 cannot be closed for now.

@brianteeman
Copy link
Copy Markdown
Contributor Author

No not yet there is still more components

@richard67
Copy link
Copy Markdown
Member

@brianteeman There is also a column checked_out_time in the #__banner_clients table. Shouldn't that be done with this PR, too?

@wilsonge wilsonge merged commit 58744a8 into joomla:4.0-dev Jul 17, 2019
@wilsonge
Copy link
Copy Markdown
Contributor

Thanks!

@brianteeman
Copy link
Copy Markdown
Contributor Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants