Skip to content

[4.0] Contact - nullable datetime fields#24675

Merged
wilsonge merged 20 commits intojoomla:4.0-devfrom
alikon:patch-87
May 16, 2019
Merged

[4.0] Contact - nullable datetime fields#24675
wilsonge merged 20 commits intojoomla:4.0-devfrom
alikon:patch-87

Conversation

@alikon
Copy link
Copy Markdown
Contributor

@alikon alikon commented Apr 22, 2019 •
edited
Loading

Pull Request for Issue #16788 ....same as #21901 for modules

Summary of Changes

some datetime fields should be nullable as default

  • publish_up
  • publish_down
  • checked_out_time

Testing Instructions

test com_contacts

Expected result

contacts should works as before

@alikon alikon mentioned this pull request Apr 22, 2019
Closed
zero-24
zero-24 previously requested changes Apr 22, 2019
View reviewed changes
@ghost ghost changed the title [4.0][com_contact] nullable datetime fields [4.0] Contact - nullable datetime fields Apr 22, 2019
@alikon alikon marked this pull request as ready for review April 22, 2019 09:42
@zero-24 zero-24 dismissed their stale review April 22, 2019 09:43

Issues seams to be solved

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Apr 22, 2019

Open question (can see it both ways). Should modified date by nullable or should enforce it being date time (modified date is the created date on creation).

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented Apr 23, 2019

yes, to me too, we need to take a decision and make it consistently across the cms

@twister65
Copy link
Copy Markdown
Contributor

twister65 commented May 5, 2019

I have tested this item ✅ successfully on 44b90c5

with MySQL and PostgreSQL.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24675.

@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented May 5, 2019 •
edited
Loading

Unless anyone feels strongly I'd rather have this set all the time. So modified date is always set (to the created date time when the article is created)

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 7, 2019

ok modified field is not null now

p.s.
i hope prepared statement should be room for a new pr

Quy
Quy reviewed May 7, 2019
View reviewed changes
@alikon
Update administrator/components/com_contact/Table/ContactTable.php
c5ae141 
Co-Authored-By: alikon <optimus4joomla@gmail.com>
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented May 7, 2019

There's no queries here that require prepared statements. This looks fine. Just needs some tests creating and updating contacts

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 8, 2019

are you sure ?
just for example components/com_contact/Model/CategoryModel.php is full of places where we can use prepared statements

joomla-cms/components/com_contact/Model/CategoryModel.php

Line 193 in c5ae141

$query->where('(a.name LIKE ' . $search . ')');

joomla-cms/components/com_contact/Model/CategoryModel.php

Line 170 in c5ae141

$query->where('a.published = ' . (int) $state);

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 15, 2019

I have tested this item ✅ successfully on 456218b

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24675.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 15, 2019

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24675.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 15, 2019
@zero-24 zero-24 added this to the Joomla 4.0 milestone May 16, 2019
@wilsonge wilsonge merged commit 9087899 into joomla:4.0-dev May 16, 2019
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented May 16, 2019

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 16, 2019
@alikon alikon deleted the patch-87 branch May 16, 2019 17:41
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Jun 28, 2019
@brianteeman
[4.0] Baanners - nullable datetime fields
63f9ca2 
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 mentioned this pull request Jun 28, 2019
Merged
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Jun 28, 2019
@brianteeman
[4.0] newsfeeds - nullable datetime fields
0bd703f 
Pull Request for Issue joomla#16788 ....same as joomla#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_newsfeeds

### Expected result
newsfeeds should works as before
@brianteeman brianteeman mentioned this pull request Jun 28, 2019
Merged
@alikon alikon mentioned this pull request Jul 9, 2019
Closed
wilsonge pushed a commit that referenced this pull request Jul 17, 2019
@brianteeman @wilsonge
[4.0] Banners - nullable datetime fields (#25360)
58744a8 
Pull Request for Issue #16788 ....same as #24675 for contacts

Summary of Changes
some datetime fields should be nullable as default

- publish_up
- publish_down
- checked_out_time
wilsonge pushed a commit that referenced this pull request Jul 23, 2019
@brianteeman @wilsonge
[4.0] newsfeeds - nullable datetime fields (#25362)
e0ef559 
Pull Request for Issue #16788 ....same as #24675 for contacts - I really just copied the work of others (someone has to)

some datetime fields should be nullable as default

- publish_up
- publish_down
- checked_out_time
This was referenced Sep 21, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Quy Quy Quy left review comments

+1 more reviewer

@zero-24 zero-24 zero-24 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla 4.0

Development

Successfully merging this pull request may close these issues.

7 participants

@alikon @wilsonge @twister65 @Quy @zero-24 @joomla-cms-bot @SniperSister